Re-structure styles and theme switching components#32679
Conversation
Co-authored-by: Georgia Monahan <38015950+grgia@users.noreply.github.com>
…:margelo/expensify-app-fork into @chrispader/theme-switching-eslint-rules
…e-switching-eslint-rules
…e-switching-eslint-rules
…ructure-styles-and-theme-switching
|
Hey! I see that you made changes to our Form component. Make sure to update the docs in FORMS.md accordingly. Cheers! |
This comment was marked as outdated.
This comment was marked as outdated.
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! 🧪🧪 |
|
If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel! Web- Chat - Placeholder "Write something..." is not centered in Message ComposerAction Performed:
Expected Result:"Write something..." should be centered in Message Composer Actual Result:Placeholder "Write something..." is not centered in Message Composer Platforms:Which of our officially supported platforms is this issue occurring on?
Version Number: 1.4.11-1 Reproducible in staging?: No Reproducible in production?: No If this was caught during regression testing, add the test name, ID and link from TestRail: Email or phone of affected tester (no customers): Logs: https://stackoverflow.com/c/expensify/questions/4856 Notes/Photos/Videos: Any additional supporting documentation Recording.705.mp4Expensify/Expensify Issue URL: Issue reported by: Applause - Internal Team Slack conversation: |
|
If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel! Web - Notification - User does not receive notification when new message arriveAction Performed:
Expected Result:User A should receive notification for all new messages Actual Result:User does not receive notification when new message arrive Platforms:Which of our officially supported platforms is this issue occurring on?
Version Number: 1.4.11-1 Reproducible in staging?: No Reproducible in production?: No If this was caught during regression testing, add the test name, ID and link from TestRail: Email or phone of affected tester (no customers): Logs: https://stackoverflow.com/c/expensify/questions/4856 Notes/Photos/Videos: Any additional supporting documentation Bug6310782_1702404096233.Recording__1545.1.mp4Expensify/Expensify Issue URL: Issue reported by: Applause - Internal Team Slack conversation: |
|
If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel! IOU- User able to select email id while creating distance requestAction Performed:
Expected Result:For distance request, workspace only user should be allowed to select. When user search for email, no results found message must be displayed. User should be allowed to search and select only workspace while making distance request Actual Result:When user search for email, results are shown and user is allowed to select email for making distance request. After selecting email, on tapping request, a distance request is created with red dot and unexpected error message .Opening distance request created displays receipt missing details with red dot is shown in request preview Platforms:Which of our officially supported platforms is this issue occurring on?
Version Number: 1.4.11-1 Reproducible in staging?: No Reproducible in production?: No If this was caught during regression testing, add the test name, ID and link from TestRail: Email or phone of affected tester (no customers): Logs: https://stackoverflow.com/c/expensify/questions/4856 Notes/Photos/Videos: Any additional supporting documentation Bug6310797_1702404779857.full_video.mp4Expensify/Expensify Issue URL: Issue reported by: Applause - Internal Team Slack conversation: |
|
If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel! IOU-Split bill created in announce room is not displayedAction Performed:
Expected Result:Split bill created in announce room must be displayed Actual Result:Split bill created in announce room is not displayed Platforms:Which of our officially supported platforms is this issue occurring on?
Version Number: 1.4.11-1 Reproducible in staging?: No Reproducible in production?: No If this was caught during regression testing, add the test name, ID and link from TestRail: Email or phone of affected tester (no customers): Logs: https://stackoverflow.com/c/expensify/questions/4856 Notes/Photos/Videos: Any additional supporting documentation Bug6310980_1702417152330.announce.mp4Expensify/Expensify Issue URL: Issue reported by: Applause - Internal Team Slack conversation: |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! 🧪🧪
|
|
retrying desktop ^ |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! 🧪🧪 |
grgia
left a comment
There was a problem hiding this comment.
We've done a full regression suite and have addressed or dismissed all bugs linked in this PR.
Lint is failing due to unused styles in style util files that are never touched. I don't think we should remove these in this PR so I will merge with these lint errors as they are non-blocking/not necessary for these files.
This comment was marked as resolved.
This comment was marked as resolved.
|
@grgia looks like this was merged without a test passing. Please add a note explaining why this was done and remove the |
|
^ |
|
🚀 Deployed to staging by https://github.com/grgia in version: 1.4.13-0 🚀
|
|
🚀 Deployed to staging by https://github.com/grgia in version: 1.4.13-0 🚀
|
|
🚀 Deployed to production by https://github.com/jasperhuangg in version: 1.4.13-8 🚀
|

@grgia
Details
This PR implements a new file/folder structure for styles and especially theme switching components.
The
src/stylesfolder now follows the following structure:theme: all themes, colors and theme-dependent items for styling like illustrations.context: contexts used for theme switchingillustrations: central definition of all theme-dependent illustrationsthemes: all illustration lists for different themesthemes: different theme color palettescolors.ts: all colors used across different themestypes.ts: types used for theme switchingindex.ts: central definition of all available themesutils: all utility functions for styling.index.ts: contains generator function forStyleUtils, which will contain all of the style utils and be the only location from which we should use utils (viauseStyleUtilshook, finished in a follow-up PR)index.ts: all static but theme-dependent styles (used viauseThemeStyleshook)Multiple components have been moved to different locations for consistency:
useTheme,useThemeStyles,useStyleUtils,useThemeIllustrations,useThemePreferenceanduseThemePreferenceWithStaticOverridehave all been moved tosrc/hooks.ThemeProvider,ThemeStylesProviderandThemeIllustrationsProviderhave been moved tosrc/components.withTheme,withThemeStyles,withStyleUtilsanduseThemeIllustrationshave been moved tosrc/components.Fixed Issues
$ #32664
PROPOSAL:
Tests
Full Regression Suite was done on this PR by QA
Offline tests
QA Steps
NO QA
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(theme.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
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
MacOS: Desktop