-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
[Theme Switching Migration] Navigation + SignIn + WholeApp Batch #31723
Changes from 4 commits
1b22f59
8004be7
9941bb7
49d261f
fce17cd
94613ec
a898ce0
f5d38a5
e93fb3d
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 |
---|---|---|
|
@@ -16,7 +16,7 @@ import useKeyboardState from '@hooks/useKeyboardState'; | |
import useNetwork from '@hooks/useNetwork'; | ||
import useWindowDimensions from '@hooks/useWindowDimensions'; | ||
import * as Browser from '@libs/Browser'; | ||
import styles from '@styles/styles'; | ||
import useThemeStyles from '@styles/useThemeStyles'; | ||
import toggleTestToolsModal from '@userActions/TestTool'; | ||
import CONST from '@src/CONST'; | ||
import {defaultProps, propTypes} from './propTypes'; | ||
|
@@ -44,6 +44,7 @@ const ScreenWrapper = React.forwardRef( | |
) => { | ||
const {windowHeight, isSmallScreenWidth} = useWindowDimensions(); | ||
const {initialHeight} = useInitialDimensions(); | ||
const styles = useThemeStyles(); | ||
const keyboardState = useKeyboardState(); | ||
const {isDevelopment} = useEnvironment(); | ||
const {isOffline} = useNetwork(); | ||
|
@@ -59,14 +60,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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. any reason for this change? 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. Mr. Linter insisted |
||
onPanResponderRelease: toggleTestToolsModal, | ||
}), | ||
).current; | ||
|
||
const keyboardDissmissPanResponder = useRef( | ||
PanResponder.create({ | ||
onMoveShouldSetPanResponderCapture: (e, gestureState) => { | ||
onMoveShouldSetPanResponderCapture: (_e, gestureState) => { | ||
const isHorizontalSwipe = Math.abs(gestureState.dx) > Math.abs(gestureState.dy); | ||
const shouldDismissKeyboard = shouldDismissKeyboardBeforeClose && isKeyboardShown && Browser.isMobile(); | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,15 +1,9 @@ | ||
import {CardStyleInterpolators, createStackNavigator} from '@react-navigation/stack'; | ||
import React from 'react'; | ||
import _ from 'underscore'; | ||
import styles from '@styles/styles'; | ||
import useThemeStyles from '@styles/useThemeStyles'; | ||
import SCREENS from '@src/SCREENS'; | ||
|
||
const defaultSubRouteOptions = { | ||
cardStyle: styles.navigationScreenCardStyle, | ||
headerShown: false, | ||
cardStyleInterpolator: CardStyleInterpolators.forHorizontalIOS, | ||
}; | ||
|
||
/** | ||
* Create a modal stack navigator with an array of sub-screens. | ||
* | ||
|
@@ -20,6 +14,14 @@ function createModalStackNavigator(screens) { | |
const ModalStackNavigator = createStackNavigator(); | ||
|
||
function ModalStack() { | ||
const styles = useThemeStyles(); | ||
|
||
const defaultSubRouteOptions = { | ||
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. also in favour of memoizing here |
||
cardStyle: styles.navigationScreenCardStyle, | ||
headerShown: false, | ||
cardStyleInterpolator: CardStyleInterpolators.forHorizontalIOS, | ||
}; | ||
|
||
return ( | ||
<ModalStackNavigator.Navigator screenOptions={defaultSubRouteOptions}> | ||
{_.map(screens, (getComponent, name) => ( | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. shouldn't 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.
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. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. 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. gochya! |
||
<Stack.Screen | ||
name="Settings" | ||
component={ModalStackNavigators.SettingsModalStackNavigator} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,12 +1,17 @@ | ||
import {CardStyleInterpolators} from '@react-navigation/stack'; | ||
import styles from '@styles/styles'; | ||
|
||
const RHPScreenOptions = { | ||
/** | ||
* RHP stack navigator screen options generator function | ||
* @function | ||
* @param {Object} styles - The styles object | ||
* @returns {Object} - The screen options object | ||
*/ | ||
const RHPScreenOptions = (styles) => ({ | ||
headerShown: false, | ||
animationEnabled: true, | ||
gestureDirection: 'horizontal', | ||
cardStyle: styles.navigationScreenCardStyle, | ||
cardStyleInterpolator: CardStyleInterpolators.forHorizontalIOS, | ||
}; | ||
}); | ||
|
||
export default RHPScreenOptions; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -152,7 +152,7 @@ function SignInPageLayout(props) { | |
</View> | ||
) : ( | ||
<ScrollView | ||
contentContainerStyle={scrollViewContentContainerStyles} | ||
contentContainerStyle={scrollViewContentContainerStyles(styles)} | ||
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. We should memoize |
||
keyboardShouldPersistTaps="handled" | ||
ref={scrollViewRef} | ||
> | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,4 @@ | ||
import styles from '@styles/styles'; | ||
|
||
// On web, we can use flex to fit the content to fit the viewport within a ScrollView. | ||
const scrollViewContentContainerStyles = styles.flex1; | ||
const scrollViewContentContainerStyles = (styles) => styles.flex1; | ||
|
||
export default scrollViewContentContainerStyles; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,7 +1,5 @@ | ||
import styles from '@styles/styles'; | ||
|
||
// Using flexGrow on mobile allows the ScrollView container to grow to fit the content. | ||
// This is necessary because making the sign-in content fit exactly the viewheight causes the scroll to stop working on mobile. | ||
const scrollViewContentContainerStyles = styles.flexGrow1; | ||
const scrollViewContentContainerStyles = (styles) => styles.flexGrow1; | ||
|
||
export default scrollViewContentContainerStyles; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,12 +2,13 @@ import React from 'react'; | |
import Text from '@components/Text'; | ||
import TextLink from '@components/TextLink'; | ||
import withLocalize, {withLocalizePropTypes} from '@components/withLocalize'; | ||
import styles from '@styles/styles'; | ||
import useThemeStyles from '@styles/useThemeStyles'; | ||
import CONST from '@src/CONST'; | ||
|
||
const linkStyles = [styles.textExtraSmallSupporting, styles.link]; | ||
|
||
function Terms(props) { | ||
const styles = useThemeStyles(); | ||
const linkStyles = [styles.textExtraSmallSupporting, styles.link]; | ||
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. same here |
||
|
||
return ( | ||
<Text style={[styles.textExtraSmallSupporting, styles.mb4]}> | ||
{props.translate('termsOfUse.phrase1')} | ||
|
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.
should we memoize this one?
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.
nah, not worth it. No perf gains, as there's no serious computation involved here
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.
I believe we should
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lukemorawski could you address this comment?
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.
@fabioh8010 I still don't understand why I should memoize this :)
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.
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..