-
Notifications
You must be signed in to change notification settings - Fork 110
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
feat: add a 'Scroll up for tutorial' button after scrolling past the instructions #2288
feat: add a 'Scroll up for tutorial' button after scrolling past the instructions #2288
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.
Thanks! This looks great. The animation is exactly what I had in mind.
The one problem I found was that it does not work when the keyboard is up on mobile. This might be related to position: fixed
on mobile Safari. I had the same problem with the toolbar. I had to switch it position: absolute
and update top
on scroll, but it's very choppy unfortunately.
The other small issue is that there is a dead zone where the tutorial buttons are off screen, but the scroll up button is not shown. Instead, the scroll up button should be shown as soon as the Next/Prev buttons are no longer visible. I think we need a small offset to account for the padding in between the bottom edge of the Next/Prev buttons and the bottom edge of the tutorial
div.
2231978
to
bbb8780
Compare
…S keyboard is open by switching to position: absolute
…om getBoundingClientRect
…ay of the 'Scroll up' button
…scrolling by having the scrollPosition update without a transition
@raineorshine -- This is ready for review again, when you're available. Thanks!
Good find. After testing, I noticed that the iOS Safari viewport messes with both the Quite unfortunate, but it seems the only solution compatible with iOS was to drop As you suggested, switching to I used CSS
We could fix this with a hardcoded offset for the padding, but I'd rather avoid using a magic number which may vary from platform to platform, or over time as the app styling could change. Instead, I've switched the visibility Demo: RPReplay_Final1724139035.mp4 |
80019f7
to
10b9d7c
Compare
Thank you @raineorshine -- Ready again for review 🎉 |
|
||
/** A button to scroll up to the tutorial. */ | ||
const TutorialScrollUpButton: FC<{ show: boolean }> = ({ show }) => { | ||
const scrollTop = scrollTopStore.useState() |
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.
For performance reasons, let's ensure the component is only re-rendered on scroll when visible.
You might try something like this:
const scrollTop = scrollTopStore.useSelector(state => show ? 0 : state)
However my brief testing shows that the component is still getting re-rendered on scroll, so it may be coming from a parent also. Maybe the component needs React.memo
?
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.
Rendering when scrolling and not-visible is necessary for the animation of the component. If we don't position it correctly before/after it being shown, then it does not transition to/from the correct location.
Changing the value of scrollTop
to be null
with that selector does not prevent the scrollTopStore
from still triggering the re-render, and React.memo
can only help with props
, not the renders triggered within the component by the store.
Fortunately, I don't believe this should cause any worthwhile performance issues; if there were issues when the element is not displayed, then there would also be issues when the element is displayed. With the scrollTopStore
already handling throttling and this element only being rendered during the brief tutorial, pursuing this particular case may be premature optimization.
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.
That's a fair point. I'm fine leaving it as-is then.
src/hooks/useIsVisible.ts
Outdated
* @returns An array containing the visibility state and the ref object of the element to check. | ||
*/ | ||
const useIsVisible = <T extends HTMLElement>(initialValue = false): [boolean, React.RefObject<T>] => { | ||
const scrollTop = scrollTopStore.useState() |
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.
You can also use the reactMinistore's useEffect
directly:
scrollTopStore.useEffect(checkVisibility)
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.
Very cool! Retrieving the scrollTop
value only to use it as a hook dependency felt awkward, but I didn't realize scrollTopStore.useEffect
existed. That's much nicer. Refactored: 84ce5aa
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.
Yeah, there are a ton of React state management libraries out there, but honestly I've gotten so much mileage out of rolling my own ministore and react-ministore that are a mere 150 LOC combined.
Ministores can even be composed.
This pull request implements #172 by displaying a "Scroll up for tutorial" button when the user has scrolled far enough for the Tutorial component to no longer be visible. The visibility of the Tutorial component is detected using a new hook called
useIsVisible
which uses the IntersectionObserver API. The button has a slide and fade animation, and clicking it scrolls back to the top of the page to display the tutorial instructions.Demo:
Screen.Recording.2024-08-14.at.9.20.24.PM.mov