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

Fix Scrollama problems on browser resize/zoom #107

Merged
merged 7 commits into from
Oct 24, 2024
Merged

Conversation

jimjam-slam
Copy link
Collaborator

@jimjam-slam jimjam-slam changed the title Issues101-fromdev Fix Scrollama problems on browser resize/zoom Oct 20, 2024
@jimjam-slam
Copy link
Collaborator Author

Note that I thought I would need to manually reattach the Scrollama event handlers, so I moved them into named functions. I ended up just needing to call .resize() methods on the scroller objects, so the refactor into named functions ended up not being necessary. I think it improves readability and is worth keeping, but I can undo that part if you disagree!

@andrewpbray
Copy link
Contributor

Testing it locally on chrome, the freeze-on-resize issue was still there (as was some jumpiness).

ojs-test.mov

I wanted to test it on the preview site, so I reran that job and, presto, no jumpiness and the resize/reload issue is fixed! Good to know that serving that stuff locally won't behave in quite the same manner.

Alright, I'll proceed with the review!

@andrewpbray
Copy link
Contributor

Small note: in bcfcc66 I moved the second hotkey event listener inside the on-document-load after looking into it a bit a deciding that that probably makes the most sense for all of our listeners.

Copy link
Contributor

@andrewpbray andrewpbray left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

_extensions/closeread/closeread.js Show resolved Hide resolved
docs/index-styles.css Show resolved Hide resolved
@@ -2,6 +2,8 @@
// closeread.js //
//==============//

// import throttleDebounce from "https://cdn.jsdelivr.net/npm/[email protected]/+esm"

Copy link
Collaborator Author

@jimjam-slam jimjam-slam Oct 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd originally intended to use this package, but I didn't really want t bite off a bundling toolchain (although we could probably load it via a script tag in the HTML, as we do our existing dependencies). I ended up using the native browser setTimeout() instead, so this can go (although I could try loading it via script and using debounce, which would prevent .resize() being called multiple times). Not sure there's any noticeable performance impact currently, though. I'll remove this commented out code for now.

@jimjam-slam jimjam-slam merged commit aca6319 into dev Oct 24, 2024
1 check passed
@jimjam-slam jimjam-slam deleted the issues101-fromdev branch October 24, 2024 23:23
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

Successfully merging this pull request may close these issues.

2 participants