[Theme Switching Migration] Navigation + SignIn + WholeApp Batch #31723
Conversation
| const styles = useThemeStyles(); | ||
| const theme = useTheme(); | ||
|
|
||
| const types = { |
There was a problem hiding this comment.
should we memoize this one?
There was a problem hiding this comment.
nah, not worth it. No perf gains, as there's no serious computation involved here
There was a problem hiding this comment.
Why? The computation of types is not very resource-intensive and the component does re-render frequently. Memoizing types might not yield a significant performance improvement and could even be slightly less efficient due to the overhead of the useMemo hook itself.
There was a problem hiding this comment.
@lukemorawski could you address this comment?
There was a problem hiding this comment.
@fabioh8010 I still don't understand why I should memoize this :)
There was a problem hiding this comment.
i also think we should memoize this. Memoization will just compare by reference here and isn't adding much overhead. This is a fairly large object we're re-instatiating every render..
| const styles = useThemeStyles(); | ||
| const theme = useTheme(); | ||
|
|
||
| const types = { |
| @@ -63,14 +64,14 @@ const ScreenWrapper = React.forwardRef( | |||
|
|
|||
| const panResponder = useRef( | |||
| PanResponder.create({ | |||
| onStartShouldSetPanResponderCapture: (e, gestureState) => gestureState.numberActiveTouches === CONST.TEST_TOOL.NUMBER_OF_TAPS, | |||
| onStartShouldSetPanResponderCapture: (_e, gestureState) => gestureState.numberActiveTouches === CONST.TEST_TOOL.NUMBER_OF_TAPS, | |||
There was a problem hiding this comment.
any reason for this change?
There was a problem hiding this comment.
Mr. Linter insisted
| @@ -26,7 +26,7 @@ function RightModalNavigator(props) { | |||
| <NoDropZone> | |||
| {!isSmallScreenWidth && <Overlay onPress={props.navigation.goBack} />} | |||
| <View style={styles.RHPNavigatorContainer(isSmallScreenWidth)}> | |||
| <Stack.Navigator screenOptions={RHPScreenOptions}> | |||
| <Stack.Navigator screenOptions={RHPScreenOptions(styles)}> | |||
There was a problem hiding this comment.
shouldn't RHPScreenOptions(styles) be memoized?
There was a problem hiding this comment.
RHPScreenOptions is only tightly coupled with styles so I see no gain in memoizing it, but maybe I'm missing something.
There was a problem hiding this comment.
it's basically like doing
<Stack.Navigator screenOptions={{
headerShown: false,
animationEnabled: true,
gestureDirection: 'horizontal',
cardStyle: styles.navigationScreenCardStyle,
cardStyleInterpolator: CardStyleInterpolators.forHorizontalIOS,
}} />There was a problem hiding this comment.
I'm not certain but I think Rory mentioned about memoization around those functions passing styles because of frequent updates he noticed, and passing those to the navigator will refresh all screens I believe.
| const statusBarAnimation = useSharedValue(0); | ||
|
|
||
| // https://reactnavigation.org/docs/themes | ||
| const navigationTheme = { |
There was a problem hiding this comment.
This, I understand why :)
| @@ -152,7 +152,7 @@ function SignInPageLayout(props) { | |||
| </View> | |||
| ) : ( | |||
| <ScrollView | |||
| contentContainerStyle={scrollViewContentContainerStyles} | |||
| contentContainerStyle={scrollViewContentContainerStyles(styles)} | |||
There was a problem hiding this comment.
We should memoize scrollViewContentContainerStyles(styles)
|
@fabioh8010 friendly bump up! |
|
@lukemorawski my comments are still there 🙂 Did you have a chance to take a look? |
|
@fabioh8010 Yes, I've responded with some questions. Please have a look :)
|
|
@lukemorawski could you try merging main for performance tests? |
There was a problem hiding this comment.
@lukemorawski I think we may be missing two from the original list
- ModalStackNavigators.js,
- NavigationRoot.js,
- Terms.js
- GrowlNotification/index.js
- getRootNavigatorScreenOptions.js
- src/libs/Navigation/AppNavigator/getRootNavigatorScreenOptions.js
- src/libs/Navigation/AppNavigator/Navigators/CentralPaneNavigator/BaseCentralPaneNavigator.js
- src/libs/Navigation/AppNavigator/RHPScreenOptions.js
- src/libs/Navigation/AppNavigator/getRootNavigatorScreenOptions.js
- src/pages/signin/SignInPageLayout/signInPageStyles/index.native.js
- src/pages/signin/SignInPageLayout/signInPageStyles/index.js
- src/components/ScreenWrapper/index.js
|
@grgia on it |
|
@grgia ModalStackNavigator is refactored and getRootNavigatorScreenOptions.js was already done. |
|
@lukemorawski I think modalStackNavigator still needs updating to use the hook, for example here - |
|
@grgia sorry, perhaps I made myself not clear, but I have refactored ModalStackNavigator per your request :) |
|
oops my bad, thank you! 🙏 |
|
@burczu PR should ready for a review :) |
|
please resolve the conflicts :) |
There was a problem hiding this comment.
LGTM.
We might want to discuss the conventions about memoization on a broader scale? @grgia
(When to use useMemo and useCallback, and in which cases not)
| const styles = useThemeStyles(); | ||
| const theme = useTheme(); | ||
|
|
||
| const types = { |
There was a problem hiding this comment.
i also think we should memoize this. Memoization will just compare by reference here and isn't adding much overhead. This is a fairly large object we're re-instatiating every render..
| @@ -20,6 +14,14 @@ function createModalStackNavigator(screens) { | |||
| const ModalStackNavigator = createStackNavigator(); | |||
|
|
|||
| function ModalStack() { | |||
| const styles = useThemeStyles(); | |||
|
|
|||
| const defaultSubRouteOptions = { | |||
There was a problem hiding this comment.
also in favour of memoizing here
src/pages/signin/Terms.js
Outdated
| function Terms(props) { | ||
| const styles = useThemeStyles(); | ||
| const linkStyles = [styles.textExtraSmallSupporting, styles.link]; |
|
please resolve conflicts :) |
Reviewer Checklist
Screenshots/VideosAndroid: NativeAndroid: mWeb ChromeiOS: NativeiOS: mWeb SafariMacOS: Chrome / SafariMacOS: Desktop |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to production by https://github.com/mountiny in version: 1.4.6-2 🚀
|
Details
This PR refactors the code to use dynamic theme/styling object, by using
useThemeStylesanduseThemehooks in favor of static imports, to enable app wide theme switching.Fixed Issues
$ #31684
PROPOSAL: no proposal
Tests
Offline tests
same as QA
QA Steps
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)myBool && <MyComponent />.src/languages/*files and using the translation methodWaiting for Copylabel for a copy review on the original GH to get the correct copy.STYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
no screenshots requiredAndroid: mWeb Chrome
no screenshots required
iOS: Native
no screenshots required
iOS: mWeb Safari
no screenshots required
MacOS: Chrome / Safari
no screenshots required
MacOS: Desktop
no screenshots required