[No QA] Move static values from dynamicStyles - #70986
Conversation
|
Hey! I see that you made changes to our Form component. Make sure to update the docs in FORMS.md accordingly. Cheers! |
| // For more information about these differences and how to test in development mode, | ||
| // see`Expensify/App/contributingGuides/APPLE_GOOGLE_SIGNIN.md` | ||
| CONFIG.ENVIRONMENT !== CONST.ENVIRONMENT.DEV && ( | ||
| CONFIG.ENVIRONMENT == CONST.ENVIRONMENT.DEV && ( |
There was a problem hiding this comment.
Are you sure about this change?
There was a problem hiding this comment.
Great point, thanks! I overlooked this one
| }, | ||
| }) satisfies StaticStyles; | ||
|
|
||
| const dynamicStyles = (theme: ThemeColors) => |
There was a problem hiding this comment.
I'm wondering if it wouldn't be worthwhile to add StyleSheet.create for all dynamic styles, or at least for some of them whose props don't change very often?
The advantage of this solution is that we gain additional style validation. However, I'm not sure it will be optimal for React Native for Web. On the web, StyleSheet.create changes styles to util classNames (similar to tailwind), and I'm concerned about styles that attach to props like width and height (this could generate a lot of classes).
What are your thoughts on this?
function createStyleSheet<T extends ViewStyle | TextStyle | ImageStyle>(styles: T): T {
return StyleSheet.create({obj: styles}).obj;
}
const dynamicStyles = (theme: ThemeColors) =>
createStyleSheet({
topLevelNavigationTabBar: (shouldDisplayTopLevelNavigationTabBar: boolean, shouldUseNarrowLayout: boolean, bottomSafeAreaOffset: number) => ({
// We have to use position fixed to make sure web on safari displays the bottom tab bar correctly.
// On natives we can use absolute positioning.
position: Platform.OS === 'web' ? 'fixed' : 'absolute',
opacity: shouldDisplayTopLevelNavigationTabBar ? 1 : 0,
pointerEvents: shouldDisplayTopLevelNavigationTabBar ? 'auto' : 'none',
width: shouldUseNarrowLayout ? '100%' : variables.sideBarWithLHBWidth,
paddingBottom: bottomSafeAreaOffset,There was a problem hiding this comment.
And please do not treat this comment as a review of this PR, but more as a follow-up suggestion.
There was a problem hiding this comment.
The solution you've proposed looks pretty neat, thanks! At this point I'm trying to incrementally improve the whole style management, so your idea could be a good follow-up!
Nevertheless I'm not sure which direction we're going to take yet - I'm even considering removal of dynamicStyles at some point, since most of them are used only in one place and have just 1-2 dynamic values. In this case they could be solved by a simple ternary expression. Other dynamic styles could be moved to utils and become more generic.
All in all I like your idea, and depending on the way we're going to take we may incline to use your solution as well 🚀
staszekscp
changed the title
dynamicStylesdynamicStyles
|
@jayeshmangwani Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
|
@staszekscp Since our initial PR #70261 was reverted here #70099 (comment), should we first fix that issue #70983 and then move forward with this PR? |
|
Yes, I've published a revert with the fix already so I hope it won't take long |
mountiny
changed the title
dynamicStylesdynamicStyles
mountiny
changed the title
dynamicStylesdynamicStyles
|
This is off hold now, could you please merge main? |
|
Thanks for merging main! I’m running through the checklist now so we don’t run into more conflicts. |
…sion-labs/expensify-app-fork into chore/migrate-static-styles
|
I love when on Tuesday morning a test fails on CI, even though it passes locally |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
| width: '100%', | ||
| height: '100%', | ||
| opacity: isActive ? 1 : 0, | ||
| transition: 'opacity 0.2s ease-in', |
There was a problem hiding this comment.
NAB: Should we add this transition: 'opacity 0.2s ease-in' as a static style?
| top: 0, | ||
| bottom: 0, | ||
| right: hasMarginRight ? variables.sideBarWidth : 0, | ||
| backgroundColor: theme.overlay, |
There was a problem hiding this comment.
NAB: Maybe we can add backgroundColor: theme.overlay to the static styles?
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb Chromemweb-chrome.moviOS: HybridAppiOS.moviOS: mWeb Safarimweb-safari.movMacOS: Chrome / Safariweb.movMacOS: Desktopdesktop.mov |
jayeshmangwani
left a comment
jayeshmangwani
left a comment
There was a problem hiding this comment.
Changes look good to me 🚀
|
Fair point with the styles you've found! |
mountiny
left a comment
mountiny
left a comment
There was a problem hiding this comment.
Thank you, could not find anything weird there so going to move it ahead
|
✋ 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 staging by https://github.com/mountiny in version: 9.2.21-0 🚀
|
|
🚀 Deployed to production by https://github.com/Julesssss in version: 9.2.21-4 🚀
|
|
🚀 Deployed to production by https://github.com/Julesssss in version: 9.2.21-4 🚀
|
6 participants
cc: @mountiny
Explanation of Change
This PR moves static values from
dynamicStyles, so they can be wrapped inStyleSheet.create.Fixed Issues
$ #70099
PROPOSAL:
Tests
Offline tests
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.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
Screen.Recording.2025-09-23.at.09.09.59.mov
Android: mWeb Chrome
Screen.Recording.2025-09-23.at.09.29.02.mov
iOS: Native
Screen.Recording.2025-09-23.at.15.32.04.mov
iOS: mWeb Safari
Screen.Recording.2025-09-23.at.15.34.18.mov
MacOS: Chrome / Safari
Screen.Recording.2025-09-23.at.08.54.53.mov
MacOS: Desktop
Screen.Recording.2025-09-23.at.08.58.53.mov