This repository has been archived by the owner on Jan 5, 2023. It is now read-only.
fix(Carousel): Use event handler instead of useEffect for scroll #683
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
useEffect was causing problems on React 18, so this change reworks the scrolling mechanism to explicitly call `scrollTo` on every single manual event (clicks, keyboard) as well as when the `currentIndex` prop changes. It also sets `isManuallyUpdating` to true. After that we listen to the 'scroll' event and reset `isManuallyUpdating` to false after the very last scroll event. The intersection observer is used for syncing the active dot with scroll events that are triggered natively (e.g. dragging on mobile). We check for `isManuallyUpdating` here to avoid messing with manual `scrollTo` calls.
mayank99
requested review from
a team,
veekeys and
elephantcatdog
and removed request for
a team
mayank99
commented
May 20, 2022
Comment on lines
83
to
85
| scrollTimeout.current = window.setTimeout(() => { | ||
| isManuallyUpdating.current = false; | ||
| }, 100); |
There was a problem hiding this comment.
This is the only part that I'm not sure about. I had to do this because there is no scrollend event. To make things worse, React doesn't support passive scroll events, which is the recommended approach.
But at least the delay doesn't cause any usability problems (in my testing).
There was a problem hiding this comment.
Wouldnt IntersectionObserver help here? You could know, when element is fully visible (so assume scoll has ended)
There was a problem hiding this comment.
how would it work? we already use intersection observers and this code is directly interacting with it
veekeys
reviewed
May 23, 2022
Comment on lines
83
to
85
| scrollTimeout.current = window.setTimeout(() => { | ||
| isManuallyUpdating.current = false; | ||
| }, 100); |
There was a problem hiding this comment.
Wouldnt IntersectionObserver help here? You could know, when element is fully visible (so assume scoll has ended)
veekeys
approved these changes
May 24, 2022
mayank99
added a commit
to iTwin/iTwinUI
that referenced
this pull request
Dec 21, 2022
…in/iTwinUI-react#683) useEffect was causing problems on React 18, so this change reworks the scrolling mechanism to explicitly call scrollTo on every single manual event (clicks, keyboard) as well as when the currentIndex prop changes. It also sets isManuallyUpdating to true. And then we listen to the 'scroll' event and reset isManuallyUpdating to false after the very last scroll event. The intersection observer is used for syncing the active dot with scroll events that are triggered natively (e.g. dragging on mobile). We check for isManuallyUpdating here to avoid messing with manual scrollTo calls.
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
useEffect was causing problems on React 18 (#656 (review)), so this change reworks the scrolling mechanism to explicitly call
scrollToon every single manual event (clicks, keyboard) as well as when thecurrentIndexprop changes. It also setsisManuallyUpdatingto true.After that we listen to the 'scroll' event and reset
isManuallyUpdatingto false after the very last scroll event.The intersection observer is used for syncing the active dot with scroll events that are triggered natively (e.g. dragging on mobile). We check for
isManuallyUpdatinghere to avoid messing with manualscrollTocalls.