search

NUKnightLab / TimelineJS3

TimelineJS v3: A Storytelling Timeline built in JavaScript. http://timeline.knightlab.com
Mozilla Public License 2.0
2.99k stars 621 forks source link

Timeline component is missing the teardown flow #742

Open oleksandr-danylchenko opened 2 years ago

oleksandr-danylchenko commented 2 years ago

We develop a SPA web app and there appeared a need to dynamically replace the theme of the Timeline.

If we pass the different a theme prop, e.g. "contrast", to the options, the component will load the "contrast" theme CSS file. But then we'll not be able to get back to the normal theme, because the "contrast" stylesheet is stuck in the DOM. It would be much better if there was an ability to delete all the previously downloaded styles from the DOM.

A similar thing happens with the different script tags that are loaded as part of the Media, e.g. Imgur. When the TL is mounted, it loads the Imgur script and appends it to the DOM. If I try to render the same Timeline again, the Imgur script will be added the second time to the DOM.

https://user-images.githubusercontent.com/68850090/173536604-2c88c34d-0f62-4bdd-9706-f5c81c598698.mp4

Therefore, it would be great to have some public remove method on the Timeline class that will remove all the downloaded assets from the DOM when is about to get unmounted. This removal can still be manual and the teardown responsibility can lay on the library consumers. I think it'll give more control over how the TL is rendered.

JoeGermuska commented 2 years ago

These are valid points; however, the vast majority of TimelineJS users use the simple hosted embed from our CDN, where these are non-issues.

Since we don't generally consider TimelineJS under active development, we're unlikely to prioritize developing it here.

We'd be receptive to PRs with test cases that implement this behavior, if someone is motivated to develop them. (Note that any remove method would, ideally, take into account the possibility that there are other timelines on a page, although that seems ugly in every path that comes to mind for managing it.)

oleksandr-danylchenko commented 2 years ago

however, the vast majority of TimelineJS users use the simple hosted embed from our CDN, where these are non-issues.

That makes sense to me. Even if the Timeline is used under the iframe, all the stylesheets will be gone as soon as the iframe will get unmounted.

that seems ugly in every path that comes to mind for managing it.

That, unfortunately, true 😢. I tried to update the Loader class in our local fort but realized that would take too much time right now to consider and test all the possible cases. So, for now, I decided on our app side to implement the dynamic replacement of the theme stylesheet on CSS level without unmounting or cleaning the Timeline. Still in progress...