swup / scroll-plugin

A swup plugin for smooth scrolling 🏄‍♂️
https://swup.js.org/plugins/scroll-plugin
MIT License
3 stars 8 forks source link

Refactor and cleanup #32

Closed hirasso closed 2 years ago

hirasso commented 2 years ago

With a fresh mind I did some cleanup and tested it again. Works great for me!

While adding more JSDoc to the code, I noticed this in unmount():

window.history.scrollRestoration = 'auto';

This should not be hardcoded, since the scrollRestoration could have been already altered before the plugin was initialized. So I saved the previous value and reset it in unmount() like so:

window.history.scrollRestoration = this.previousScrollRestoration;

I hope it's ok that this became part of this PR.

daun commented 2 years ago

I love the added doc comments, we should look into slowly adding those to the core as well and start generating type definitions.

hirasso commented 2 years ago

Ready for a new version I would say! 🍾

daun commented 2 years ago

Feel free to create a PR for the version bump! We still have to add the version bump action to all the plugins.

daun commented 2 years ago

Will this be a breaking change? Since we're restoring a different value for scroll restoration. Or we wait until we figure out scrolling when animateHistoryBrowsing is enabled and bump the major version then.

hirasso commented 2 years ago

No, this is not introducing breaking changes. The scroll position of the next (link-click) page is always being reset, if not overwritten by shouldResetScrollPosition. Let's handle animateHistoryBrowsing in the next release.

hirasso commented 2 years ago

Sems like I can't run the workflow here?

Screen Shot 2022-08-19 at 13 06 45

daun commented 2 years ago

Perfect, let's bump minor now and major if we ever tackle animated history browsing.

daun commented 2 years ago

Yeah, the workflow only exists on the core repo for now. You need to create a PR after running npm version.