-
Notifications
You must be signed in to change notification settings - Fork 327
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
Use IntersectionObserver to detect current page #3682
Use IntersectionObserver to detect current page #3682
Conversation
What problem is this fixing? |
If this works, it should solve an issue at #3694 |
|
||
const pageRef = useRef(); | ||
|
||
const rootMargin = '-50% 0px'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you explain the behavior of how rootMargin
works here? Any difference between this and threshold
instead?
Maybe just add a brief comment explaining how this boundary determines the current page.
useEffect(()=>{ | ||
const observer = new IntersectionObserver( | ||
([entry])=>{ | ||
const pageNo = parseInt(entry.target.getAttribute('id').slice(1)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we can just use the component's index
prop directly instead of reading it from the DOM target?
if(pageRef.current == null) return; | ||
observer.unobserve(pageRef.current); | ||
}; | ||
}, [pageRef.current, rootMargin]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Neither of these values trigger a redraw when changed, so they won't have any effect here in the useEffect
dependency array. The effect will just fire once when the page component is mounted and then never again. In that case, this is equivalent to just an empty array []
.
@@ -50,8 +50,42 @@ const fetchThemeBundle = async (obj, renderer, theme)=>{ | |||
})); | |||
}; | |||
|
|||
let pageArray = []; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the purpose of the pageArray
instead of simply setting the current page directly from the page that triggered the intersectionObserver
event?
Seems we could get away with a simple getPage
and setPage
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My thinking was that - particularly with the new BrewRenderer toolbar - that there can be multiple pages visible in the view port, so an array of page numbers is more appropriate than a single value.
This should be superseded by PR #3845. |
Any reason you didn't close it then? |
This PR uses the IntersectionObserver to detect which page elements are in the viewport and thus visible to the end user. This data is updated as the user scrolls, and is passed to a new function
updatePageArray
inhelpers.js
.This PR also adds two further new functions to
helpers.js
:getPageNumber
: returns an integer, the average value of the content of thepageArray
which should be the most central page in the range of visible pagesgetVisiblePageRange
: returns a string, if the array length is 3 or less, joined by commas (e.g.1, 2, 3
) and for lengths greater than 3, the first and last page separated by a dash (e.g.1 - 5
).