search

bespokejs / bespoke

DIY Presentation Micro-Framework
http://markdalgleish.com/projects/bespoke.js/
MIT License
4.68k stars 443 forks source link

Destroying the presentation from 3rd party code. #50

Closed neochief closed 7 years ago

neochief commented 9 years ago

First of all, thank you very much for this great library.

I've been using it for my project, which involves showing multiple presentations on one page. It looks like an index of presentations, user can play them in sequential order and I'd like to make it in very seamless way, without page reload. So far, everything was good, but I noticed that some plugins like bespoke-hash bind some handlers to the window object events, and don't unbind them when bespoke instance gets destroyed (and later replaced with a new one). And there's no clear way for such plugins to unbind their handlers, since there's no destroy event in the core library.

I propose to add the destroy() method, which will fire the 'destroy' event prior to the presentation element removal. This way all plugin developers would know that there's a standard event, which they could handle and implement the clean-up.

If you find this reasonable and decide to accept this (or similar) code, I promise to create pull requests for all popular bespoke plugins with destroy handlers implementations.

Thanks!

coveralls commented 9 years ago

Coverage Status

Coverage decreased (-6.67%) when pulling f9b81cce80d449120864deb127759d125743cf1c on neochief:destroy into 45b2c6e1937e42092e49ed0f3299e2a8ed3670d7 on markdalgleish:master.

markdalgleish commented 9 years ago

I think this is a great idea. Would you be able to add tests for this?

Also worth considering here whether it would make sense to return false from inside a destroy event handler. Nothing jumps to mind for me.

mojavelinux commented 9 years ago

I've been preparing for this change by implementing a destroy event handler in each of my plugins. The destroy event handler unbinds any global event listeners that would otherwise get left behind.

Here's an example: https://github.com/opendevise/bespoke-cursor/blob/master/lib/bespoke-cursor.js#L18-L20

mojavelinux commented 9 years ago

It's also important to note that this makes testing life easier.

mojavelinux commented 9 years ago

I think the method should have a void return value. I'm also not convinced that we need to pass the active slide. I guess it doesn't hurt, but it's not really the subject of this particular event. I think it should be a no-args event.

mojavelinux commented 7 years ago

I've integrated this suggestion. However, I decided not to have core remove the node from the DOM. That should be something that a listener does. Core just fires the event. I decided to send the current slide as payload as that could be useful when cleaning up the DOM. After the event is fired, I unregister all listeners (honoring the nature of destroy).

mojavelinux commented 7 years ago

See https://github.com/bespokejs/bespoke/commit/fbc6363b086dc304be8ffa06aa62c280de8bfbd8