-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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 screen jump from Welcome screen to Demo screen #2744
Merged
Merged
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
lindboe
force-pushed
the
lindboe/fix-screen-jump
branch
from
August 14, 2024 21:51
19ab614
to
4a8473b
Compare
Android and iOS were both jumping when navigating from the Welcome screen to the Demo Showroom screen (although in different directions, and Android was less severe). I found this issue was prevented when we use the `<StatusBar>` component on the WelcomeScreen as well as DemoShowroom, so converted WelcomeScreen to use the built-in `Screen` component, which includes `<StatusBar>`. However, that raised another issue: when navigating from the Login screen to the Welcome screen, the "fixed" `Screen` component on Welcome screen would render the wrong height; it would be slightly too tall. This didn't occur when changing presets and reloading the app via fast refresh, which indicated to me that it was likely due to a state change that was using the "old" screen dimensions from Login screen, but then was incorrect after another state change took place on the Welcome screen. I found that was correct: the `useHeader` hook was using useEffect, which means that header height wouldn't be set until the second render, so when the KeyboardAvoidingView on on Welcome screen tried to render, it had the wrong height information and rendered the wrong size. By using `useLayoutEffect`, we make sure the header height is set before any of the screen components try to render, fixing the issue.
causes a lint error in a generated project, so fixed
lindboe
force-pushed
the
lindboe/fix-screen-jump
branch
from
August 14, 2024 21:59
4a8473b
to
bdacbe0
Compare
frankcalise
approved these changes
Aug 14, 2024
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.
Great investigation! So much smoother
markrickert
reviewed
Aug 14, 2024
React.useEffect(() => { | ||
// To avoid a visible header jump when navigating between screens, we use | ||
// `useLayoutEffect`, which will apply the settings before the screen renders. | ||
React.useLayoutEffect(() => { |
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.
YES!
markrickert
approved these changes
Aug 15, 2024
🎉 This PR is included in version 10.0.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Please verify the following:
yarn test
jest tests pass with new tests, if relevantyarn lint
eslint checks pass with new code, if relevantyarn format:check
prettier checks pass with new code, if relevantREADME.md
(or relevant documentation) has been updated with your changesDescribe your PR
To fix Android build issue, depends on #2745.
Android and iOS were both jumping when navigating from the Welcome screen to the Demo Showroom screen (although in different directions, and Android was less severe).
I found this issue was prevented when we use the
StatusBar
component on the WelcomeScreen as well as DemoShowroom, so converted WelcomeScreen to use the built-inScreen
component, which includesStatusBar
.However, that raised another issue: when navigating from the Login screen to the Welcome screen, the "fixed"
Screen
component on Welcome screen would render the wrong height; it would be slightly too tall.The
useHeader
hook was using useEffect, which means that header height wouldn't be set until the second render, so when the KeyboardAvoidingView on on Welcome screen tried to render, it had the wrong height information and rendered the wrong size.By using
useLayoutEffect
, we make sure the header height is set before any of the screen components try to render, fixing the issue.AndroidJumpBefore.mp4
AndroidJumpAfter.mp4
ScreenJumpIOSBefore.mp4
ScreenJumpiOSAfter.mp4