-
Notifications
You must be signed in to change notification settings - Fork 7
Fix ScrollView
scroll position issues (#501)
#657
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
Conversation
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.
Pull Request Overview
Fixes ScrollView so it correctly updates its scroll position when content or viewport size changes, and prevents arrow controls from getting stuck after rapid scrolling.
- Refactored
useScrollPosition
hook: switched fromuseLayoutEffect
touseEffect
, dropped the externaldependencies
argument, and locked its dependency array. - Updated
ScrollView
component: memoized the scroll state handler, switched to functional state updates, and added aResizeObserver
to re-evaluate scroll edges on dimension changes. - Adjusted hook usage and cleaned up state logic.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
File | Description |
---|---|
src/components/ScrollView/_hooks/useScrollPositionHook.js | Swapped useLayoutEffect for useEffect , removed the dependencies parameter, and locked dependencies to [] . |
src/components/ScrollView/ScrollView.jsx | Wrapped handleScrollViewState in useCallback , refactored state setters, updated hook invocation, and added a ResizeObserver effect. |
Comments suppressed due to low confidence (3)
src/components/ScrollView/_hooks/useScrollPositionHook.js:7
- The hook signature was changed by removing the
dependencies
parameter, which is an API-breaking change. Please update any consumers and document the new parameter order in the release notes.
export const useScrollPosition = (effect, contentEl, viewportEl, wait) => {
src/components/ScrollView/ScrollView.jsx:181
- [nitpick] Consider adding tests for the new ResizeObserver logic to verify that style or window size changes correctly trigger scroll position and arrow state updates.
const resizeObserver = new ResizeObserver(() => {
src/components/ScrollView/_hooks/useScrollPositionHook.js:18
- Switching from useLayoutEffect to useEffect may cause layout measurements to occur after paint, leading to flicker or incorrect scroll positioning. Consider using useLayoutEffect for DOM reads and registration of scroll listeners before paint.
useEffect(() => {
I performed tests in major browsers on MacOS and it works as expected. It would be nice if the end arrow would not flicker during the autoscroll. It always appears for fraction of second and it then disappears. But if implemented incorrectly, we may end up when state then it is not scrolled at the end but with hidden end arrow (e.g. during manual scroll intervention during autoscrolling). I think we can stick with current solution unless you think you have some simple solution. |
Couldn't think of a clean and simple solution. |
ScrollView would not update if the size of the content changed due to style or window size changes. ScrollView would get stuck in a state where the arrows would not update when the user scrolled fast back and forth.
da0ade4
to
03b2897
Compare
ScrollView would not update if the size of the content changed due to style or window size changes.
ScrollView would get stuck in a state where the arrows would not update when the user scrolled fast back and forth.