-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
Merged
grgia
merged 9 commits into
Expensify:main
from
lukemorawski:31677-theme_switching_navigation_signin_wholeapp
Nov 29, 2023
Merged
Changes from all commits
Commits
Show all changes
9 commits
Select commit
Hold shift + click to select a range
1b22f59
refactorign/theme switching
lukemorawski 8004be7
fix/memoized navigationTheme
lukemorawski 9941bb7
added ModalStackNavigator
lukemorawski 49d261f
Merge branch 'main' into 31677-theme_switching_navigation_signin_whol…
lukemorawski fce17cd
great memoization
lukemorawski 94613ec
Merge branch 'main' into 31677-theme_switching_navigation_signin_whol…
lukemorawski a898ce0
navigation root fix
lukemorawski f5d38a5
more memoization
lukemorawski e93fb3d
linting
lukemorawski File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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(); | ||
|
||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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; |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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; |
4 changes: 1 addition & 3 deletions
4
src/pages/signin/SignInPageLayout/signInPageStyles/index.native.js
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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; |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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..