Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Destroying plugins that create DOM elements outside of plugin selector #2

Open
jwlawrence opened this issue Aug 21, 2013 · 3 comments

Comments

@jwlawrence
Copy link

I've been trying to come up with a solution for destroying plugins that wrap the original selector with additional markup. The saveClone method only accounts for changes made within the original selector. I'm going to play around with saving the state of the parent node as well and checking to see if any changes have been made within, but I was wondering if anyone's looked into this already or if there are plans to add support for this feature.

@DesignByOnyx
Copy link
Owner

mmm, I have tried to create handlers so that you could add your own custom functionality for this kind of stuff. My first inclination is that an plugin which is adding nodes outside of the original node is a bit backwards in their logic. They should instead be wrapping the contents with an inner node - which is quite common for animations and stuff like that:

<!-- original markup -->
<div class="my-component">
   <a>...</a>
   <b>...</b>
   <footer>...</footer>
</div>

Then you do something like $('.my-component').responsivePlugin(...); and you might get:

<div class="my-component">
   <div class="added-for-animation">
       <a>...</a>
       <b>...</b>
       <footer>...</footer>
    </div>
</div>

If your plugin is adding extra markup outside our component, then you should use the callbackAfter option to do some final cleanup:

$('.my-component').responsivePlugin({
    pluginName: "some-plugin",
    destroyMethodName: "destroy",
    breakpointOptions: {
        "only screen and (min-width: 48em)": {
            options: {},
            callbackAfter: function() {
                // remember this === $('.my-component')
                this.parent().replaceWith(this);
            }
        }
    }
});

@jwlawrence
Copy link
Author

Totally agree with you about the extra nodes outside of the original selector being backwards. In fact, I nuked the plugin and am just rolling my own solution. This is something that's likely to come up from time to time though, so I thought it was worth noting.

Correct me if I'm wrong, but the callbackAfter will fire only on a breakpoint match, not an unmatch event. So in the example above the the plugin would initialize adding the extra markup and then the callbackAfter function would effectively remove it, which would likely break the plugin's functionality. Would it make sense to add a callbackCleanup callback that would fire on unmatch? Again, I've just done a quick read through of your plugin, so I could be totally off here.

Also, unrelated, but I noticed a number of references to "destryMethodName" which seems to be a typo for "destroyMethodName". Maybe i'm missing something though.

Thanks for your response and taking the time to look into this.

@DesignByOnyx
Copy link
Owner

Holy crap! I had a super-bad typo in the code which I have just [embarrassingly] discovered. I have updated it - please pull the latest version. Also, you are correct in that my last example will not work for you. Instead, try passing a function to the "destroyMethodName" option:

$('.my-component').responsivePlugin({
    pluginName: "some-plugin",
    destroyMethodName: function() {
        // Check to make sure the parentNode is what you expect before moving forward...
        if( this.parent().hadClass('foo') )
            this.parent().replaceWith(this);
    }
    breakpointOptions: {
        "only screen and (min-width: 48em)": {}
    }
});

I am not opposed to changing the API for this either and am open for suggestions. Maybe a set of callbacks is in order:

  • beforeDestroy
  • afterDestroy
  • beforeDomRestore
  • afterDomRestore
  • beforeInstantiate
  • afterInstantiate

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants