Theme switching: Add Theme to User Preferences (No QA) #21666

chrispader · 2023-06-27T11:03:04Z

Details

Fixed Issues

$ #21020
PROPOSAL: GH_LINK_ISSUE(COMMENT)

Tests

Verify that no errors appear in the JS console

We decided to extract the code for the menu entry for "Color theme" in Settings -> Preferences into a separate PR, in order to enable theme switching for the user, once every component is migrated to the new theme switching logic.

That's why in order to test the changes of THIS PR, we'll need to manually add the changes for the menu entry from the other PR, to test the new screen.

Copy over code minimum required demo code from this demo PR

In the PreferencesPage

Add this to below the MenuItemWithTopDescription of preferredLocale:

<MenuItemWithTopDescription
    shouldShowRightIcon
    title={"Mock theme"}
    description={"Theme"}
    onPress={() => Navigation.navigate(ROUTES.SETTINGS_THEME)}
/>

Uncomment the Settings_Preferences_Theme entry in ModalStackNavigators.js:

const SettingsModalStackNavigator = createModalStackNavigator([
    ...
    // UNCOMMENT THIS

    // {
    //     getComponent: () => {
    //         const SettingsPreferencesThemePage = require('../../../pages/settings/Preferences/ThemePage').default;
    //         return SettingsPreferencesThemePage;
    //     },
    //     name: 'Settings_Preferences_Theme',
    // },
    ...
])

Go to Settings -> Preferences
Click on the menu "Theme" menu entry
Make sure you get redirected to a new "Theme" page
Change the theme (click on one of the not selected themes)
Make sure that a request with the name "UpdateTheme" is sent to the backend
After the request is done, make sure the "preferredTheme" value changes in Onyx (Dev Tools -> Application -> IndexedDB -> OnyxDB -> keyvaluepairs)
Make sure that the current theme value changed accordingly (in ThemePage and PreferencesPage) and the app's theme updates too

Offline tests

No additional tests needed.

QA Steps

Verify that no errors appear in the JS console

No QA, this PR creates the theme settings page but does not yet add it to the app.

PR Author Checklist

Screenshots/Videos

Web

Mobile Web - Chrome

Mobile Web - Safari

Desktop

iOS

Android

src/ROUTES.js

src/languages/es.js

src/libs/actions/App.js

src/pages/settings/Preferences/ColorThemePage.js

grgia

First pass review! I realize this is a WIP but here ya go

src/ONYXKEYS.js

src/ROUTES.js

src/languages/en.js

src/libs/actions/App.js

src/pages/settings/Preferences/ColorThemePage.js

src/ONYXKEYS.js

melvin-bot · 2023-07-05T10:41:49Z

@sobitneupane 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]

chrispader · 2023-07-05T11:19:08Z

@sobitneupane please ignore this mention, @grgia is gonna review this

grgia · 2023-07-05T14:57:11Z

@chrispader we need to add the header text and device settings options

here's the mockup again:

src/pages/settings/Preferences/ThemePage.js

chrispader · 2023-07-11T09:28:41Z

Small bug following the QA steps - this will more effect this PR- https://github.com/Expensify/App/pull/21669/files

but if we connect to ONYX and the preferred theme is '', it will override the default theme and cause an error trying to access it heretitle={themes[props.preferredTheme].label} let's make sure we override with the default after connecting to onyx

i.e title={themes[props.preferredTheme || CONST.THEME.DEFAULT].label}

I understand your proposal, but this seems to not solve the root problem, does it? the value from Onyx may never be '', right?

It should always either be undefined - in which case there is a defaultProp defined - or have a value of either dark/light/system.

Maybe i'm wrong here though..

grgia

Screen.Recording.2023-07-11.at.10.26.20.AM.mov

Is there a reason we aren't saving the selection as part of this PR? Did you intend to add that to the going live?

grgia · 2023-07-11T09:30:23Z

I understand your proposal, but this seems to not solve the root problem, does it? the value from Onyx may never be '', right?
It should always either be undefined - in which case there is a defaultProp defined - or have a value of either dark/light/system.

@chrispader no, the ONYX key may not be set and will return '', in which case we default as if the user selected system

chrispader · 2023-07-11T09:32:16Z

Is there a reason we aren't saving the selection as part of this PR? Did you intend to add that to the going live?

Yes, you suggested to put the saving/persisting logic into a separate PR here.

Should i add it here instead?

grgia · 2023-07-11T09:59:53Z

I clicked on one and they all selected? Any clue as to why?

chrispader · 2023-07-11T11:58:37Z

I clicked on one and they all selected? Any clue as to why?

This should be resolved

grgia · 2023-07-11T13:29:32Z

Reviewer Checklist

I have verified the author checklist is complete (all boxes are checked off).
I verified the correct issue is linked in the ### Fixed Issues section above
I verified testing steps are clear and they cover the changes made in this PR
- I verified the steps for local testing are in the Tests section
- I verified the steps for Staging and/or Production testing are in the QA steps section
- I verified the steps cover any possible failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
- I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
I checked that screenshots or videos are included for tests on all platforms
I included screenshots or videos for tests on all platforms
I verified tests pass on all platforms & I tested again on:
- Android / native
- Android / Chrome
- iOS / native
- iOS / Safari
- MacOS / Chrome / Safari
- MacOS / Desktop
If there are any errors in the console that are unrelated to this PR, I either fixed them (preferred) or linked to where I reported them in Slack
I verified proper code patterns were followed (see Reviewing the code)
- I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick).
- I verified that the left part of a conditional rendering a React component is a boolean and NOT a string, e.g. myBool && <MyComponent />.
- I verified that comments were added to code that is not self explanatory
- I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
- I verified any copy / text shown in the product is localized by adding it to src/languages/* files and using the translation method
- I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
- I verified any copy / text that was added to the app is grammatically correct in English. It adheres to proper capitalization guidelines (note: only the first word of header/labels should be capitalized), and is approved by marketing by adding the Waiting for Copy label for a copy review on the original GH to get the correct copy.
- I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
- I verified the JSDocs style guidelines (in STYLE.md) were followed
If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
I verified that this PR follows the guidelines as stated in the Review Guidelines
I verified other components that can be impacted by these changes have been tested, and I retested again (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar have been tested & I retested again)
I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
If a new component is created I verified that:
- A similar component doesn't exist in the codebase
- All props are defined accurately and each prop has a /** comment above it */
- The file is named correctly
- The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
- The only data being stored in the state is data necessary for rendering and nothing else
- For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
- Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
- All JSX used for rendering exists in the render method
- The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
If any new file was added I verified that:
- The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
If a new CSS style is added I verified that:
- A similar style doesn't already exist
- The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
If the PR modifies code that runs when editing or sending messages, I tested and verified there is no unexpected behavior for all supported markdown - URLs, single line code, code blocks, quotes, headings, bold, strikethrough, and italic.
If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.
I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

Screenshots/Videos

Web

Mobile Web - Chrome

chrome not working on my emulator

Mobile Web - Safari

Desktop

iOS

Android

src/pages/settings/Preferences/ThemePage.js

src/libs/Navigation/AppNavigator/ModalStackNavigators.js

src/libs/actions/User.js

Co-authored-by: Georgia Monahan <[email protected]>

grgia

Woot woot!

OSBotify · 2023-07-12T05:25:47Z

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

OSBotify · 2023-07-13T02:00:00Z

🚀 Deployed to staging by https://github.com/grgia in version: 1.3.40-0 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

bernhardoj · 2023-07-13T09:38:06Z

src/libs/actions/User.js

+    );
+
+    Navigation.navigate(ROUTES.SETTINGS_PREFERENCES);
+}


@chrispader hi, I think we want to replace the navigate with goBack, so we won't experience this kind of issue in the future.

I'm currently working on a PR to do this fix, but I can't access the theme page yet, so I'm leaving a note here so you can fix it in the next PR. Thanks!

@bernhardoj the ThemePage should now be available :)

@chrispader I don't see it on the Preferences page, or is it on other page

https://github.com/Expensify/App/blob/main/src/pages/settings/Preferences/ThemePage.js

cc @bernhardoj

Ah, I mean the page on the app, not the code 😅. If I manually create a button to navigate there, QA won't be able to test it.

ah yea, makes sense.This is the PR that's gonna add the button, but it's gonna take some more time until we merge it, because we have to migrate the whole app to theme switching first

Got it. Maybe you can apply this change replace the navigate with goBack in your PR?

bernhardoj · 2023-07-13T09:38:20Z

src/pages/settings/Preferences/ThemePage.js

+            <HeaderWithBackButton
+                title={props.translate('themePage.theme')}
+                shouldShowBackButton
+                onBackButtonPress={() => Navigation.navigate(ROUTES.SETTINGS_PREFERENCES)}


Same as above

@chrispader if you haven't already applied these two changes to your PR, could you add an issue to the light mode project view to make sure we don't forget this?

OSBotify · 2023-07-14T14:44:56Z

🚀 Deployed to production by https://github.com/Julesssss in version: 1.3.40-5 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

Christoph Pader added 4 commits June 27, 2023 12:53

add preferences for color theme

f08cbec

add color theme CONST

369a9cd

add color theme ONYXKEYS

f65fde0

fix ColorThemePage

e452835

grgia reviewed Jun 27, 2023

View reviewed changes

src/ROUTES.js Outdated Show resolved Hide resolved

remove menu entry for color theme page

b9851a7

chrispader changed the title ~~Theme switching: Add Theme to User Preferences and Enable Theme Switching~~ Theme switching: Add Theme to User Preferences Jun 27, 2023

grgia reviewed Jun 27, 2023

View reviewed changes

src/languages/es.js Outdated Show resolved Hide resolved

grgia reviewed Jun 27, 2023

View reviewed changes

src/libs/actions/App.js Outdated Show resolved Hide resolved

grgia reviewed Jun 27, 2023

View reviewed changes

src/pages/settings/Preferences/ColorThemePage.js Outdated Show resolved Hide resolved

Christoph Pader added 2 commits July 4, 2023 12:08

Merge branch 'main' into @chrispader/theme-switching-preferences

1691b1b

fix: import

e7f0471

chrispader mentioned this pull request Jul 4, 2023

Theme switching: Going Live! (Add preferences menu entry for ThemePage) #21669

Merged

56 tasks

grgia reviewed Jul 5, 2023

View reviewed changes

Christoph Pader and others added 6 commits July 5, 2023 10:56

Merge branch 'main' into @chrispader/theme-switching-preferences

6ed9344

update naming

d7d7606

update comment

aabee1e

wrap ThemePage in ScrollView

91582bc

fix wrong import

320b140

remove ScrollView and fix link

8171f11

chrispader marked this pull request as ready for review July 5, 2023 10:41

chrispader requested a review from a team as a code owner July 5, 2023 10:41

melvin-bot bot requested review from sobitneupane and removed request for a team July 5, 2023 10:41

Christoph Pader added 3 commits July 7, 2023 13:57

Merge branch 'main' into @chrispader/theme-switching-preferences

6b6a04b

update translations

e7fdfe1

update translations structure

e018a50

grgia reviewed Jul 11, 2023

View reviewed changes

src/pages/settings/Preferences/ThemePage.js Outdated Show resolved Hide resolved

grgia reviewed Jul 11, 2023

View reviewed changes

chrispader added 2 commits July 11, 2023 11:33

default to "CONST.THEME.DEFAULT" in case onyx key is set to "''"

0a4a43e

add "persisting" logic

b94ee71

chrispader added 2 commits July 11, 2023 12:09

fix persisting logic

0b98862

disable themePage temporarily

deb324c

grgia reviewed Jul 11, 2023

View reviewed changes

src/pages/settings/Preferences/ThemePage.js Outdated Show resolved Hide resolved

grgia reviewed Jul 11, 2023

View reviewed changes

src/libs/Navigation/AppNavigator/ModalStackNavigators.js Show resolved Hide resolved

grgia reviewed Jul 11, 2023

View reviewed changes

src/libs/actions/User.js Outdated Show resolved Hide resolved

chrispader and others added 3 commits July 11, 2023 19:51

Update src/libs/Navigation/AppNavigator/ModalStackNavigators.js

de96d80

Co-authored-by: Georgia Monahan <[email protected]>

Update src/libs/actions/User.js

d73e358

Co-authored-by: Georgia Monahan <[email protected]>

remove unused import

345ad0f

grgia removed the request for review from sobitneupane July 12, 2023 05:16

grgia approved these changes Jul 12, 2023

View reviewed changes

grgia merged commit 1003476 into Expensify:main Jul 12, 2023

grgia changed the title ~~Theme switching: Add Theme to User Preferences~~ Theme switching: Add Theme to User Preferences (No QA) Jul 12, 2023

OSBotify mentioned this pull request Jul 13, 2023

Deploy Checklist: New Expensify 2023-07-13 #22783

Closed

53 tasks

bernhardoj reviewed Jul 13, 2023

View reviewed changes

bernhardoj mentioned this pull request Jul 13, 2023

Fix some RHP pages doing a NAVIGATE action instead of go back #22789

Merged

58 tasks

melvin-bot bot mentioned this pull request Jul 14, 2023

Add Theme Page UI #21020

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Theme switching: Add Theme to User Preferences (No QA) #21666

Theme switching: Add Theme to User Preferences (No QA) #21666

chrispader commented Jun 27, 2023 •

edited by grgia

Loading

grgia left a comment

melvin-bot bot commented Jul 5, 2023

chrispader commented Jul 5, 2023

grgia commented Jul 5, 2023

chrispader commented Jul 11, 2023

grgia left a comment

grgia commented Jul 11, 2023

chrispader commented Jul 11, 2023

grgia commented Jul 11, 2023

chrispader commented Jul 11, 2023

grgia commented Jul 11, 2023 •

edited

Loading

grgia left a comment

OSBotify commented Jul 12, 2023

OSBotify commented Jul 13, 2023

bernhardoj Jul 13, 2023

chrispader Jul 15, 2023

bernhardoj Jul 17, 2023

chrispader Jul 17, 2023 •

edited

Loading

bernhardoj Jul 17, 2023

chrispader Jul 18, 2023

bernhardoj Jul 18, 2023

bernhardoj Jul 13, 2023

grgia Jul 25, 2023

OSBotify commented Jul 14, 2023

Theme switching: Add Theme to User Preferences (No QA) #21666

Theme switching: Add Theme to User Preferences (No QA) #21666

Conversation

chrispader commented Jun 27, 2023 • edited by grgia Loading

Details

Fixed Issues

Tests

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

grgia left a comment

Choose a reason for hiding this comment

melvin-bot bot commented Jul 5, 2023

chrispader commented Jul 5, 2023

grgia commented Jul 5, 2023

chrispader commented Jul 11, 2023

grgia left a comment

Choose a reason for hiding this comment

grgia commented Jul 11, 2023

chrispader commented Jul 11, 2023

grgia commented Jul 11, 2023

chrispader commented Jul 11, 2023

grgia commented Jul 11, 2023 • edited Loading

Reviewer Checklist

Screenshots/Videos

grgia left a comment

Choose a reason for hiding this comment

OSBotify commented Jul 12, 2023

OSBotify commented Jul 13, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrispader Jul 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OSBotify commented Jul 14, 2023

chrispader commented Jun 27, 2023 •

edited by grgia

Loading

grgia commented Jul 11, 2023 •

edited

Loading

chrispader Jul 17, 2023 •

edited

Loading