search

ConnorAtherton / walkway

An easy way to animate SVG elements.
http://connoratherton.com/walkway
MIT License
4.37k stars 229 forks source link

destroy / cleanup method #45

Closed DanielRuf closed 4 years ago

DanielRuf commented 7 years ago

In SPAs (single page applications) and in other cases it probably makes sense to clear the current elements and remove the event listener.

At least it also makes sense to remove the event listener when not needed anymore (for example when the visibility was changed and all elements were drawn or in a destroy/cleanup method).

Also in my opinion it makes sense to add an option to disable the visibilitychange eventlistener or not add it and provide also an optional pause / pauseOnVisibilityHidden or similar method.

Your opinions on this?

ConnorAtherton commented 7 years ago

Good suggestion. I think your first two paragraphs are worthy of further treatment. I don't want to disable the visibility change listener, for reasons I have linked to in your other issue. Let me know that reasoning makes sense with you.

Note, we do (guard against)[https://github.com/ConnorAtherton/walkway/blob/v0.0.7/src/walkway.js#L115] the case where we don't need to do any work, since the document is still in view.

If we were going to do this, I think it would make sense for this to be a static method Walkway.clearAll(), or similar, since we track the elements globally to avoid setting n event listeners, instead of just 1.

DanielRuf commented 7 years ago

Just a quick additional question as npm pulled 0.0.6 (not 0.0.7, probably npm publish is needed): is the visibilitychange event listener removed after it was fired and complete called? Does not seem to be the case in the code or I oversaw something =)

A clearAll method to remove the instances and cleanup the stack sounds good. We could call it also manually if all objects are drawn and redraw and so on are not needed anymore.

DanielRuf commented 7 years ago

PS: the latest code is also not in https://cdn.jsdelivr.net/walkway/0.0.7/walkway.min.js

Diff between 0.0.6 and 0.0.7 (jsdelivr) https://www.diffchecker.com/MyBQtcsd

And it seems the redraw commit is not applied to walkway.min.js. Just wanted to mention this. And the change is also not here: https://github.com/ConnorAtherton/walkway/blob/master/walkway.min.js

PPS: the visibilitychange code also triggers instances which are not played / started, see http://codepen.io/anon/pen/eWrQbx

ConnorAtherton commented 7 years ago

We could call it also manually if all objects are drawn..

Yeah, that sounds good 👍

the visibilitychange code also triggers instances which are not played

That is for sure a bug. Perhaps we can add all these changes, update the minified bundle, and update the module correctly on npm. It looks like I just did not publish this after adding the redraw commit.

DanielRuf commented 7 years ago

And probably forgot gulp. But that is not a big deal / problem. Everyone makes mistakes sometimes and I thought I better mention that I noticed this. Better now than never I guess =)

I will test it further with the unminified version tomorrow, try to find better approaches for the visibilitychange event listener (unbind / remove after complete was called) and which elements are completed using complete() and element.animationStarted and collect the needed properties and variables which havve to be unset / cleared and event listener removed for a clearAll method.

11pm and just a few thoughts about possible solutions. Will present them in this issue tomorrow.

DanielRuf commented 7 years ago

Forgot this today, sorry. Were busy with the other projects.

Will definitely prioritize and try to do this on Monday (no job so enough time to test throughly).

DanielRuf commented 7 years ago

Worked on and solved a few things today / yesterday.

A first cleanup method for removing a single instance (people should still set their var to null and use the delete keyword as we have no control over these variables).

Improved rendering, initialization and code for the visibility eventlistener and also added the callback.

Improved the performance of a few lines. Will test and improve some more.

Added a few unobstrusive events like draw(n), complete and plan some more. The event system is just an idea and would be split into another branch afterwards. So developers could use solutions like $("svg".on("complete", function(){}) and listen to specific events.

Todo: resume animation after a cancel, go to specific frame. Includes some additional calculations.

Will present the changes this week so we can further discuss, split this into branches and PRs and merge the changes into new versions.

DanielRuf commented 7 years ago

A very rough first version with new changes and features: https://github.com/DanielRuf/walkway/blob/new-features-combined-wip/src/walkway.js

Feedback is very welcome. I will split the changes into the different feature branches when the code is ready and decided which features make sense. Also would have to add some more return values for the events.