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 Page numbers don't update when navigating with thumbnails #202

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

huytr1995
Copy link
Contributor

@huytr1995 huytr1995 commented Oct 11, 2022

Description

Ref: https://github.com/allenai/scholar/issues/33968

Since we recently are having issue with PageNumberControl due to user scrolls speed so this PR addresses this.

Reviewer Instructions

According to the video in the beginning Smita is already at page 15 but the page control is displaying it as page 6 so this can be the case that Smita scrolled faster than IntersectionObserver API picks up the change so that's why our page control can't update accordingly. So i change the threshold to 0 which mean if as soon as a tiny bit of the div is visible, IntersectionObserver will kick in.

Testing Plan

Verify when scroll manually through the page, the page number updated accordingly. Also when clicking thumbnail or TOC it will update the page number accordingly.

Output / Screenshots

Screen.Recording.2022-10-11.at.2.38.11.PM.mov

A11y

N/A

@huytr1995
Copy link
Contributor Author

@ericmarsh995 i tested by banging my mouse wheel real fast and it seem be able to keep up in page tracking correctly so i think this is it.

@ericmarsh995
Copy link
Contributor

Have you tested this on S2? Also, it might make sense to set it to 0.5 instead of 0 as most readers update the page when its halfway through

@huytr1995
Copy link
Contributor Author

huytr1995 commented Oct 12, 2022

Have you tested this on S2? Also, it might make sense to set it to 0.5 instead of 0 as most readers update the page when its halfway through

The recording is basically tested in s2 @ericmarsh995 i tried with 0.5 but its still not picking fast enough. 0 picks up faster since even with one tiny pixel can trigger intersection observer. @ericmarsh995 video is uploaded correctly now by testing in Prod.

Copy link
Contributor

@ericmarsh995 ericmarsh995 left a comment

Choose a reason for hiding this comment

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

LGTM

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