Fix: Web rendering issues and welcome screen layout issue #2809
Conversation
fpena
changed the title
| <ScrollView> | ||
| <ScreenWithoutScrolling {...props} /> | ||
| ) : ( | ||
| <ScreenWithScrolling {...props} /> | ||
| )} | ||
| </KeyboardAvoidingView> | ||
| </ScrollView> |
There was a problem hiding this comment.
I'm not sure that this is a root-cause fix. I'd like to make sure we look into what's going on and why so we know that this is where we want to fix it.
Note that in the original stack trace, the root of the error actually occurs in KeyboardAwareScrollView.
Replacing KeyboardAwareScrollView also stops the issue from occurring:
function ScreenWithScrolling(props: ScreenProps) {
...
useScrollToTop(ref)
const MyComponent = Platform.OS === "web" ? ScrollView : KeyboardAwareScrollView
return (
<MyComponent
bottomOffset={keyboardBottomOffset}
{...{ keyboardShouldPersistTaps, scrollEnabled, ref }}
Also note that on the original issue @frankcalise linked this ticket where someone noted that adding import "setimmediate" also appears to fix the issue. I tried this out and it appears to work, as well (setimmediate is apparently brought in by fbjs which is a dependency of react-native-web.
I'm not opposed to an immediate fix to unblock things, but I think we definitely need to dig deeper into this to find and fix root cause.
I looked at the error in the dev console and it gives a clearer stack trace of the issue: that internal.js file is part of react-native-keyboard-controller. its use of setImmediate makes this not compatible with web.
I think the more appropriate fix is to swap out KeyboardAwareScrollView for a regular scrollview, since it wasn't designed for web.
There was a problem hiding this comment.
In addition, I think we should probably use a const to define a new component wrapping KeyboardAwareScrollView ? ScrollView like we did with the useEffect below, and export it from a file like it's a component. that way other components that may want to use this can get a version compatible with web, instead of everyone having to write a platform ternary.
| * In mobile and also to avoid a visible header jump when navigating between screens, we use | ||
| * `useLayoutEffect`, which will apply the settings before the screen renders. | ||
| */ | ||
| const useAppropriateEffect = Platform.OS === "web" ? useEffect : useLayoutEffect |
There was a problem hiding this comment.
nit, just to make it clear what the criteria for the effect is:
| const useAppropriateEffect = Platform.OS === "web" ? useEffect : useLayoutEffect | |
| const usePlatformEffect = Platform.OS === "web" ? useEffect : useLayoutEffect |
| </KeyboardAvoidingView> | ||
| </ScrollView> | ||
| ) : ( | ||
| <KeyboardAvoidingView |
There was a problem hiding this comment.
Why do we have KeyboardAvoidingView if we also have KeyboardAwareScrollView nested within it? If we need it for the non-scrolling version, I would think:
- We wouldn't want to wrap KeyboardAvoidingView around both cases, only non-scrolling
- We wouldn't want to mix RN's KeyboardAvoidingView with RNKC's KeyboardAwareScrollView. It looks like RNKC has its own KeyboardAvoidingView.
| @@ -57,7 +57,7 @@ export const WelcomeScreen: FC<WelcomeScreenProps> = observer( | |||
| tx="welcomeScreen:readyForLaunch" | |||
| preset="heading" | |||
| /> | |||
| <Text tx="welcomeScreen:exciting" preset="subheading" /> | |||
| <Text tx="welcomeScreen:exciting" preset="subheading" style={$subheading} /> | |||
There was a problem hiding this comment.
These changes don't seem to render right for me on web:
| * therefore we want to use ScrollView just for web | ||
| */} | ||
| {Platform.OS === "web" ? ( | ||
| <ScrollView> | ||
| <ScreenWithoutScrolling {...props} /> |
There was a problem hiding this comment.
Also meant to ask: why does web only get ScreeenWithoutScrolling? Shouldn't it have both options?
I think that's why this appeared to fix things, it removes KeyboardAwareScrollView from the document.
Please verify the following:
yarn testjest tests pass with new tests, if relevantyarn linteslint checks pass with new code, if relevantyarn format:checkprettier checks pass with new code, if relevantREADME.md(or relevant documentation) has been updated with your changesDescribe your PR
Closes #2807
This PR solves 3 issues:
Screencomponent,KeyboardAvoidingViewis crashing in web. It was replaced by a normalScrollView.useHeaderhook and web,useLayoutEffectwas creating a rendering loop. It is replaced byuseEffect.Screenshots (if applicable)