-
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: Web rendering issues and welcome screen layout issue #2809
base: master
Are you sure you want to change the base?
Changes from all commits
5ddef96
3718655
ed2a520
2f1ecff
33c93f2
d2d4d3e
711f0a5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -265,19 +265,28 @@ export function Screen(props: ScreenProps) { | |
style={statusBarStyle || (themeContext === "dark" ? "light" : "dark")} | ||
{...StatusBarProps} | ||
/> | ||
|
||
<KeyboardAvoidingView | ||
behavior={isIos ? "padding" : "height"} | ||
keyboardVerticalOffset={keyboardOffset} | ||
{...KeyboardAvoidingViewProps} | ||
style={[$styles.flex1, KeyboardAvoidingViewProps?.style]} | ||
> | ||
{isNonScrolling(props.preset) ? ( | ||
{/** | ||
* KeyboardAvoidingView crashes in web, | ||
* therefore we want to use ScrollView just for web | ||
*/} | ||
{Platform.OS === "web" ? ( | ||
<ScrollView> | ||
<ScreenWithoutScrolling {...props} /> | ||
) : ( | ||
<ScreenWithScrolling {...props} /> | ||
)} | ||
</KeyboardAvoidingView> | ||
</ScrollView> | ||
Comment on lines
+273
to
+275
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 Replacing KeyboardAwareScrollView also stops the issue from occurring:
Also note that on the original issue @frankcalise linked this ticket where someone noted that adding 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 I think the more appropriate fix is to swap out There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In addition, I think we should probably use a const to define a new component wrapping |
||
) : ( | ||
<KeyboardAvoidingView | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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:
|
||
behavior={isIos ? "padding" : "height"} | ||
keyboardVerticalOffset={keyboardOffset} | ||
{...KeyboardAvoidingViewProps} | ||
style={[$styles.flex1, KeyboardAvoidingViewProps?.style]} | ||
> | ||
{isNonScrolling(props.preset) ? ( | ||
<ScreenWithoutScrolling {...props} /> | ||
) : ( | ||
<ScreenWithScrolling {...props} /> | ||
)} | ||
</KeyboardAvoidingView> | ||
)} | ||
</View> | ||
) | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,7 +9,7 @@ import { | |
import { isRTL } from "../i18n" | ||
import { useStores } from "../models" // @demo remove-current-line | ||
import { AppStackScreenProps } from "../navigators" | ||
import type { ThemedStyle } from "@/theme" | ||
import { spacing, type ThemedStyle } from "@/theme" | ||
import { useHeader } from "../utils/useHeader" // @demo remove-current-line | ||
import { useSafeAreaInsetsStyle } from "../utils/useSafeAreaInsetsStyle" | ||
import { useAppTheme } from "@/utils/useAppTheme" | ||
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
<Image | ||
style={$welcomeFace} | ||
source={welcomeFace} | ||
|
@@ -87,8 +87,9 @@ const $topContainer: ThemedStyle<ViewStyle> = ({ spacing }) => ({ | |
flexShrink: 1, | ||
flexGrow: 1, | ||
flexBasis: "57%", | ||
justifyContent: "center", | ||
justifyContent: "space-around", | ||
paddingHorizontal: spacing.lg, | ||
paddingBottom: spacing.sm, | ||
}) | ||
|
||
const $bottomContainer: ThemedStyle<ViewStyle> = ({ colors, spacing }) => ({ | ||
|
@@ -108,6 +109,10 @@ const $welcomeLogo: ThemedStyle<ImageStyle> = ({ spacing }) => ({ | |
marginBottom: spacing.xxl, | ||
}) | ||
|
||
const $subheading: TextStyle = { | ||
paddingRight: spacing.xxxl, | ||
} | ||
|
||
const $welcomeFace: ImageStyle = { | ||
height: 169, | ||
width: 269, | ||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -1,6 +1,7 @@ | ||||||
import { useLayoutEffect } from "react" | ||||||
import { useEffect, useLayoutEffect } from "react" | ||||||
import { useNavigation } from "@react-navigation/native" | ||||||
import { Header, HeaderProps } from "../components" | ||||||
import { Platform } from "react-native" | ||||||
|
||||||
/** | ||||||
* A hook that can be used to easily set the Header of a react-navigation screen from within the screen's component. | ||||||
|
@@ -13,10 +14,15 @@ export function useHeader( | |||||
deps: Parameters<typeof useLayoutEffect>[1] = [], | ||||||
) { | ||||||
const navigation = useNavigation() | ||||||
/** | ||||||
* We need to have multiple implementations of this hook for web and mobile. | ||||||
* Web needs to use useEffect to avoid a rendering loop. | ||||||
* 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit, just to make it clear what the criteria for the effect is:
Suggested change
|
||||||
|
||||||
// To avoid a visible header jump when navigating between screens, we use | ||||||
// `useLayoutEffect`, which will apply the settings before the screen renders. | ||||||
useLayoutEffect(() => { | ||||||
useAppropriateEffect(() => { | ||||||
navigation.setOptions({ | ||||||
headerShown: true, | ||||||
header: () => <Header {...headerProps} />, | ||||||
|
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.
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.