Skip to content
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

36 changes: 19 additions & 17 deletions src/components/GrowlNotification/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,26 +7,11 @@ import * as Pressables from '@components/Pressable';
import Text from '@components/Text';
import * as Growl from '@libs/Growl';
import useNativeDriver from '@libs/useNativeDriver';
import styles from '@styles/styles';
import themeColors from '@styles/themes/default';
import useTheme from '@styles/themes/useTheme';
import useThemeStyles from '@styles/useThemeStyles';
import CONST from '@src/CONST';
import GrowlNotificationContainer from './GrowlNotificationContainer';

const types = {
[CONST.GROWL.SUCCESS]: {
icon: Expensicons.Checkmark,
iconColor: themeColors.success,
},
[CONST.GROWL.ERROR]: {
icon: Expensicons.Exclamation,
iconColor: themeColors.danger,
},
[CONST.GROWL.WARNING]: {
icon: Expensicons.Exclamation,
iconColor: themeColors.warning,
},
};

const INACTIVE_POSITION_Y = -255;

const PressableWithoutFeedback = Pressables.PressableWithoutFeedback;
Expand All @@ -36,6 +21,23 @@ function GrowlNotification(_, ref) {
const [bodyText, setBodyText] = useState('');
const [type, setType] = useState('success');
const [duration, setDuration] = useState();
const styles = useThemeStyles();
const theme = useTheme();

const types = {
Copy link
Contributor

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?

Copy link
Contributor Author

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe we should

Copy link
Contributor Author

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.

Copy link
Contributor

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?

Copy link
Contributor Author

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 :)

Copy link
Contributor

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..

[CONST.GROWL.SUCCESS]: {
icon: Expensicons.Checkmark,
iconColor: theme.success,
},
[CONST.GROWL.ERROR]: {
icon: Expensicons.Exclamation,
iconColor: theme.danger,
},
[CONST.GROWL.WARNING]: {
icon: Expensicons.Exclamation,
iconColor: theme.warning,
},
};

/**
* Show the growl notification
Expand Down
7 changes: 4 additions & 3 deletions src/components/ScreenWrapper/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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();
Expand All @@ -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,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

any reason for this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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();

Expand Down
2 changes: 1 addition & 1 deletion src/libs/Navigation/AppNavigator/AuthScreens.js
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,7 @@ const defaultProps = {
function AuthScreens({isUsingMemoryOnlyKeys, lastUpdateIDAppliedToClient, session, lastOpenedPublicRoomID, demoInfo}) {
const styles = useThemeStyles();
const {isSmallScreenWidth} = useWindowDimensions();
const screenOptions = getRootNavigatorScreenOptions(isSmallScreenWidth);
const screenOptions = getRootNavigatorScreenOptions(isSmallScreenWidth, styles);
const isInitialRender = useRef(true);

if (isInitialRender.current) {
Expand Down
16 changes: 9 additions & 7 deletions src/libs/Navigation/AppNavigator/ModalStackNavigators.js
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.
*
Expand All @@ -20,6 +14,14 @@ function createModalStackNavigator(screens) {
const ModalStackNavigator = createStackNavigator();

function ModalStack() {
const styles = useThemeStyles();

const defaultSubRouteOptions = {
Copy link
Contributor

Choose a reason for hiding this comment

The 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) => (
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import {createStackNavigator} from '@react-navigation/stack';
import React from 'react';
import ReportScreenWrapper from '@libs/Navigation/AppNavigator/ReportScreenWrapper';
import getCurrentUrl from '@libs/Navigation/currentUrl';
import styles from '@styles/styles';
import useThemeStyles from '@styles/useThemeStyles';
import SCREENS from '@src/SCREENS';

const Stack = createStackNavigator();
Expand All @@ -11,6 +11,7 @@ const url = getCurrentUrl();
const openOnAdminRoom = url ? new URL(url).searchParams.get('openOnAdminRoom') : undefined;

function BaseCentralPaneNavigator() {
const styles = useThemeStyles();
return (
<Stack.Navigator>
<Stack.Screen
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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)}>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't RHPScreenOptions(styles) be memoized?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RHPScreenOptions is only tightly coupled with styles so I see no gain in memoizing it, but maybe I'm missing something.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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,
}} />

Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

gochya!

<Stack.Screen
name="Settings"
component={ModalStackNavigators.SettingsModalStackNavigator}
Expand Down
11 changes: 8 additions & 3 deletions src/libs/Navigation/AppNavigator/RHPScreenOptions.js
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
@@ -1,5 +1,4 @@
import getNavigationModalCardStyle from '@styles/getNavigationModalCardStyles';
import styles from '@styles/styles';
import variables from '@styles/variables';
import CONFIG from '@src/CONFIG';
import modalCardStyleInterpolator from './modalCardStyleInterpolator';
Expand All @@ -12,7 +11,7 @@ const commonScreenOptions = {
animationTypeForReplace: 'push',
};

export default (isSmallScreenWidth) => ({
export default (isSmallScreenWidth, styles) => ({
rightModalNavigator: {
...commonScreenOptions,
cardStyleInterpolator: (props) => modalCardStyleInterpolator(isSmallScreenWidth, false, props),
Expand Down
34 changes: 19 additions & 15 deletions src/libs/Navigation/NavigationRoot.js
Original file line number Diff line number Diff line change
@@ -1,26 +1,17 @@
import {DefaultTheme, getPathFromState, NavigationContainer} from '@react-navigation/native';
import PropTypes from 'prop-types';
import React, {useEffect, useRef} from 'react';
import React, {useEffect, useMemo, useRef} from 'react';
import {Easing, interpolateColor, runOnJS, useAnimatedReaction, useSharedValue, withDelay, withTiming} from 'react-native-reanimated';
import useCurrentReportID from '@hooks/useCurrentReportID';
import useFlipper from '@hooks/useFlipper';
import useWindowDimensions from '@hooks/useWindowDimensions';
import Log from '@libs/Log';
import StatusBar from '@libs/StatusBar';
import themeColors from '@styles/themes/default';
import useTheme from '@styles/themes/useTheme';
import AppNavigator from './AppNavigator';
import linkingConfig from './linkingConfig';
import Navigation, {navigationRef} from './Navigation';

// https://reactnavigation.org/docs/themes
const navigationTheme = {
...DefaultTheme,
colors: {
...DefaultTheme.colors,
background: themeColors.appBG,
},
};

const propTypes = {
/** Whether the current user is logged in with an authToken */
authenticated: PropTypes.bool.isRequired,
Expand Down Expand Up @@ -51,6 +42,7 @@ function parseAndLogRoute(state) {
}

function NavigationRoot(props) {
const theme = useTheme();
useFlipper(navigationRef);
const firstRenderRef = useRef(true);

Expand Down Expand Up @@ -79,10 +71,22 @@ function NavigationRoot(props) {
navigationRef.resetRoot(navigationRef.getRootState());
}, [isSmallScreenWidth, props.authenticated]);

const prevStatusBarBackgroundColor = useRef(themeColors.appBG);
const statusBarBackgroundColor = useRef(themeColors.appBG);
const prevStatusBarBackgroundColor = useRef(theme.appBG);
const statusBarBackgroundColor = useRef(theme.appBG);
const statusBarAnimation = useSharedValue(0);

// https://reactnavigation.org/docs/themes
const navigationTheme = useMemo(
() => ({
...DefaultTheme,
colors: {
...DefaultTheme.colors,
background: theme.appBG,
},
}),
[theme],
);

const updateStatusBarBackgroundColor = (color) => StatusBar.setBackgroundColor(color);
useAnimatedReaction(
() => statusBarAnimation.value,
Expand All @@ -99,12 +103,12 @@ function NavigationRoot(props) {

const animateStatusBarBackgroundColor = () => {
const currentRoute = navigationRef.getCurrentRoute();
const currentScreenBackgroundColor = (currentRoute.params && currentRoute.params.backgroundColor) || themeColors.PAGE_BACKGROUND_COLORS[currentRoute.name] || themeColors.appBG;
const currentScreenBackgroundColor = (currentRoute.params && currentRoute.params.backgroundColor) || theme.PAGE_BACKGROUND_COLORS[currentRoute.name] || theme.appBG;

prevStatusBarBackgroundColor.current = statusBarBackgroundColor.current;
statusBarBackgroundColor.current = currentScreenBackgroundColor;

if (currentScreenBackgroundColor === themeColors.appBG && prevStatusBarBackgroundColor.current === themeColors.appBG) {
if (currentScreenBackgroundColor === theme.appBG && prevStatusBarBackgroundColor.current === theme.appBG) {
return;
}

Expand Down
2 changes: 1 addition & 1 deletion src/pages/signin/SignInPageLayout/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -152,7 +152,7 @@ function SignInPageLayout(props) {
</View>
) : (
<ScrollView
contentContainerStyle={scrollViewContentContainerStyles}
contentContainerStyle={scrollViewContentContainerStyles(styles)}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should memoize scrollViewContentContainerStyles(styles)

keyboardShouldPersistTaps="handled"
ref={scrollViewRef}
>
Expand Down
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;
7 changes: 4 additions & 3 deletions src/pages/signin/Terms.js
Original file line number Diff line number Diff line change
Expand Up @@ -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];
Copy link
Contributor

Choose a reason for hiding this comment

The 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')}
Expand Down
4 changes: 3 additions & 1 deletion src/styles/styles.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3984,5 +3984,7 @@ const styles = (theme: ThemeColors) =>
const stylesGenerator = styles;
const defaultStyles = styles(defaultTheme);

type ThemeStyle = typeof defaultStyles;

export default defaultStyles;
export {stylesGenerator, type Styles};
export {stylesGenerator, type Styles, type ThemeStyle};
Loading