[Core Branding] Soften Dark Mode Colors / White by grgia · Pull Request #13583 · Expensify/App

grgia · 2022-12-14T16:15:19Z

Details

This PR modifies our default brand colors and removes the 1px right border from the LHN. Should also fix the white background behind LHN that sometimes flashes while loading the app.

Current Branch:

Fixed Issues

$ #13564

Tests

Verify that no errors appear in the JS console
Verify that the 4 modified colors are correct (LHN green, Borders, greenSupportingText, White)
Verify no white screen behind LHN while the app is loading

Offline tests

QA Steps

Verify that no errors appear in the JS console
Same as test steps

PR Author Checklist

Screenshots/Videos

Web

Mobile Web - Chrome

Mobile Web - Safari

Desktop

iOS

Android

grgia · 2022-12-14T16:27:37Z

A note, right now the border is added to the container, which I show here:

Screen.Recording.2022-12-14.at.10.49.47.AM.mov

The sidebar is composed of the Drawer Navigator, the "Chats" header and then a FlatList with all of the chats. I'm thinking there's a better way to do this so we could have the correct rounded edges without needing the right border?

grgia · 2022-12-14T17:09:07Z

Here's the current mobile/android. I think you mentioned this change being web-only, so I need to remove the border for small screen

With	Without

shawnborton · 2022-12-14T19:22:47Z

Yup that's right, let's only do the rounded borders/stroke for web.

shawnborton · 2022-12-14T19:24:55Z

In terms of the border... the idea there is that we could use the border color as a way to make it feel like the sidebar is inset from the edges of the entire app view. Alternatively, we could just give the sidebar a rounded border radius and give it an outer margin of say 8px on the top, left, and bottom. My thinking was that the border color could be an easier way to pull that off, but I am open to trying out a different method!

grgia · 2022-12-14T19:46:37Z

Oh I'm good with this method if it looks correct! My thought with my current implementation was the space between chat highlight and LHN as seen here:

One other consideration, how would this change affect desktop? Specifically in relation to the placement of these buttons:
Currently, it looks like they slightly overlap

shawnborton · 2022-12-14T19:52:45Z

My thought with my current implementation was the space between chat highlight and LHN as seen here:

Ah yeah, that's why I was thinking we could remove the right border but I suppose that's why we are getting the strange rounded shape on the top right. So maybe we should try a margin method on the outer sides, and just round the container with no border stroke?

One other consideration, how would this change affect desktop?

Oh yeah, great catch here. Can we push those buttons down and to the right a bit so they sit within the slightly lighter green part?

grgia · 2022-12-14T21:31:17Z

Here's an update on the desktop buttons:

Apparently, these are called Traffic Light Buttons

Edit:

shawnborton · 2022-12-14T21:44:10Z

Looks great on desktop... are we able to make them responsive and move back up to the top/left on smaller viewports?

grgia · 2022-12-14T22:07:29Z

Looks great on desktop... are we able to make them responsive and move back up to the top/left on smaller viewports?

I don't think we can move the traffic light buttons dynamically unless we remove the OS ones and add our own.

I didn't push the traffic light position updates, but here's the current look with using margin:

Screen.Recording.2022-12-14.at.4.52.29.PM.mov

shawnborton · 2022-12-14T22:28:00Z

Sweet, thanks for pushing up those changes!

Okay so now after seeing this (I pulled to test locally), this mis-alignment from the profile avatar in the LHN header vs the chat header is killing me:

So we could potentially change the chat header height on desktop to 73px (it's currently 65) and add 8px of paddingTop (just on desktop)... which gives us this:

But then the header of the RHN isn't aligned:

So we'd need to change the height to 73 add paddingTop: 8 there as well (just on desktop):

Thoughts? Not sure how hard that would be to pull off, the alternative option is to just ditch the rounded/inset idea altogether.

grgia · 2022-12-14T22:56:43Z

I don't mind extra work if it's worth doing!
Hmm after looking at it for a while, I think the rounded LHN looks good on its own, but it doesn't quite look right with the current square RHN/Header styles. That said, I'm happy to go forward either way :)

shawnborton · 2022-12-15T00:46:18Z

Hmm yeah, I think given that we're uncovering a lot of additional considerations for the rounded LHN, it makes sense to properly suss that out in Figma & Slack first before diving right into changes here.

So I think I would recommend we unwind those changes (sorry about that!) and then this way, this PR becomes just about color changes and not so much UI changes.

I still think it might look nice to get rid of the right border on the LHN, so maybe we can still plan to do that?

grgia · 2022-12-15T03:00:47Z

ofc np! I just committed those changes + added web screenshots to description

shawnborton · 2022-12-15T19:29:58Z

    sidebar: {
        backgroundColor: themeColors.sidebar,
        height: '100%',
+        overflow: 'hidden',


Maybe we don't need this anymore since we got rid of the border styles?

grgia · 2022-12-16T02:04:37Z

Reverted to color changes only, so opening up for review !

melvin-bot · 2022-12-16T02:05:11Z

@mananjadhav @techievivek One of you needs to 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]

techievivek · 2022-12-19T08:35:15Z

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:
- iOS / native
- Android / native
- iOS / Safari
- Android / Chrome
- MacOS / Chrome
- 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 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 correct English and 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 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.
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

Mobile Web - Safari

Desktop

iOS

Android

techievivek

Looks good, not going to wait for Manan given this is just a styling fix and Shawn has already approved it.

OSBotify · 2022-12-19T08:50:31Z

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

github-actions · 2022-12-19T09:13:17Z

Performance Comparison Report 📊

Significant Changes To Duration

There are no entries

Meaningless Changes To Duration

Show entries

Name	Duration
TTI	797.898 ms → 812.631 ms (+14.733 ms, +1.8%)
nativeLaunch	9.567 ms → 10.194 ms (+0.627 ms, +6.6%)
regularAppStart	0.016 ms → 0.014 ms (-0.002 ms, -12.3%)
runJsBundle	192.531 ms → 182.844 ms (-9.688 ms, -5.0%)

Show details

Name	Duration
TTI	Baseline Mean: 797.898 ms Stdev: 33.621 ms (4.2%) Runs: 742.3794649997726 753.1778589999303 757.629908000119 758.16917200014 762.0534479999915 762.4938470004126 763.6759350001812 772.3654119996354 773.2644629999995 774.7768599996343 777.6704339999706 781.4856449998915 789.7733429996297 793.6704850001261 795.5703320000321 795.623626999557 797.457541000098 799.925509000197 802.2690690001473 802.564442999661 803.4520030003041 803.9077540002763 818.7435259995982 826.2316680001095 827.791024999693 831.138190000318 834.0784259997308 834.2160350000486 854.2626550002024 862.5473769996315 882.4665799997747 Current Mean: 812.631 ms Stdev: 35.332 ms (4.3%) Runs: 749.2575009996071 764.4788840003312 775.6537790000439 776.388198999688 778.1327189998701 780.4562990004197 784.3716620001942 785.4908229997382 795.442850000225 797.5107319997624 799.3518599998206 800.5570799997076 801.2175099998713 804.2645439999178 805.3560490002856 808.499455999583 811.5589370001107 811.6327529996634 818.1782459998503 819.9114929996431 820.5151730002835 822.624723999761 823.612964999862 823.7801670003682 832.1832729997113 848.6800370002165 869.2103730002418 885.265122000128 885.8921330003068 899.4536610003561
nativeLaunch	Baseline Mean: 9.567 ms Stdev: 1.359 ms (14.2%) Runs: 8 8 8 8 8 8 8 9 9 9 9 9 9 9 9 9 9 9 10 10 10 10 11 11 11 11 11 12 12 13 Current Mean: 10.194 ms Stdev: 1.891 ms (18.5%) Runs: 8 8 8 8 8 8 9 9 9 9 9 9 9 9 10 10 10 10 10 10 11 11 11 11 12 12 13 13 13 14 15
regularAppStart	Baseline Mean: 0.016 ms Stdev: 0.002 ms (10.1%) Runs: 0.014322999864816666 0.014363999478518963 0.014404000714421272 0.014770000241696835 0.015056000091135502 0.015178000554442406 0.015298999845981598 0.015298999845981598 0.015463000163435936 0.01550199929624796 0.015502999536693096 0.015542999841272831 0.015544000081717968 0.01574699953198433 0.015828000381588936 0.015868999995291233 0.015949999913573265 0.01607299968600273 0.01647899951785803 0.016479999758303165 0.017129999585449696 0.01749700028449297 0.017741000279784203 0.018269999884068966 0.018352000042796135 0.018432000651955605 0.019043000414967537 0.0194089999422431 0.021199999377131462 Current Mean: 0.014 ms Stdev: 0.001 ms (4.0%) Runs: 0.013224000111222267 0.0134680001065135 0.013508999720215797 0.013712000101804733 0.013957000337541103 0.013996999710798264 0.014078999869525433 0.014078999869525433 0.014161000028252602 0.014201000332832336 0.014282000251114368 0.01428299956023693 0.014322999864816666 0.014322999864816666 0.014322999864816666 0.014403999783098698 0.014444999396800995 0.01444500032812357 0.014485999941825867 0.014486000873148441 0.014526000246405602 0.014526999555528164 0.014607000164687634 0.014851000159978867 0.014973999932408333 0.015014000236988068 0.015054999850690365 0.015381000004708767 0.016154000535607338
runJsBundle	Baseline Mean: 192.531 ms Stdev: 23.380 ms (12.1%) Runs: 156 163 163 164 164 164 166 167 179 181 184 184 185 188 189 191 191 191 194 196 196 197 205 207 209 211 216 220 222 227 240 251 Current Mean: 182.844 ms Stdev: 20.515 ms (11.2%) Runs: 157 157 159 161 163 164 167 168 171 171 172 172 173 174 175 175 176 178 178 182 184 184 185 192 201 202 207 210 210 221 228 234

mananjadhav · 2022-12-19T09:45:01Z

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:
- iOS / native
- Android / native
- iOS / Safari
- Android / Chrome
- MacOS / Chrome
- 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 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 correct English and 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 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.
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

Mobile Web - Safari

Desktop

iOS

Android

@techievivek I did review and the testing for all platforms ~~except mobile web. I am blocked on testing it due to the Auth issue and hence I didn't complete the checklist~~. Here's the completed one ~~without the mobile web~~.

mananjadhav · 2022-12-19T11:56:00Z

My IP finally worked and hence I've uploaded the screenshots for mobile web platforms too.

techievivek · 2022-12-20T04:33:34Z

@mananjadhav Aaah, sorry, I didn't realize that your IP was blocked. Thanks for reviewing this.

OSBotify · 2022-12-20T06:46:46Z

🚀 Deployed to staging by @techievivek in version: 1.2.42-0 🚀

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

OSBotify · 2022-12-21T22:57:38Z

🚀 Deployed to production by @yuwenmemon in version: 1.2.42-2 🚀

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

grgia added 2 commits December 13, 2022 16:19

Soften dark mode colors, fix white background behind LHN

46a8e19

Add rounded border around LHN

b6efde2

grgia requested a review from shawnborton December 14, 2022 16:15

grgia self-assigned this Dec 14, 2022

Make sidebar styles react to screensize, use margin instead

0f5616a

grgia added 2 commits December 14, 2022 21:54

Undo LHN changes

356fb92

Undo LHN Variables

b4b0c5d

shawnborton reviewed Dec 15, 2022

View reviewed changes

Comment thread src/styles/StyleUtils.js

shawnborton reviewed Dec 15, 2022

View reviewed changes

Keep LHN right border

fce48b9

grgia changed the title ~~[WIP][Core Branding / Dark Mode] Soften Colors/LHN~~ [WIP][Core Branding] Soften Dark Mode Colors + White Dec 16, 2022

grgia marked this pull request as ready for review December 16, 2022 02:04

grgia requested a review from a team as a code owner December 16, 2022 02:04

melvin-bot Bot requested review from mananjadhav and techievivek and removed request for a team December 16, 2022 02:05

grgia changed the title ~~[WIP][Core Branding] Soften Dark Mode Colors + White~~ [Core Branding] Soften Dark Mode Colors / White Dec 16, 2022

shawnborton approved these changes Dec 18, 2022

View reviewed changes

techievivek approved these changes Dec 19, 2022

View reviewed changes

techievivek merged commit 3d3486a into main Dec 19, 2022

techievivek deleted the georgia-soften-darkmode branch December 19, 2022 08:45

OSBotify mentioned this pull request Dec 20, 2022

Deploy Checklist: New Expensify 2022-12-20 #13723

Closed

34 tasks

Uh oh!

Conversation

grgia commented Dec 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Details

Fixed Issues

Tests

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

Uh oh!

grgia commented Dec 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

grgia commented Dec 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

shawnborton commented Dec 14, 2022

Uh oh!

shawnborton commented Dec 14, 2022

Uh oh!

grgia commented Dec 14, 2022

Uh oh!

shawnborton commented Dec 14, 2022

Uh oh!

grgia commented Dec 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

shawnborton commented Dec 14, 2022

Uh oh!

grgia commented Dec 14, 2022

Uh oh!

shawnborton commented Dec 14, 2022

Uh oh!

grgia commented Dec 14, 2022

Uh oh!

shawnborton commented Dec 15, 2022

Uh oh!

grgia commented Dec 15, 2022

Uh oh!

Uh oh!

shawnborton Dec 15, 2022

Choose a reason for hiding this comment

Uh oh!

grgia commented Dec 16, 2022

Uh oh!

melvin-bot Bot commented Dec 16, 2022

Uh oh!

techievivek commented Dec 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviewer Checklist

Screenshots/Videos

Uh oh!

techievivek left a comment

Choose a reason for hiding this comment

Uh oh!

OSBotify commented Dec 19, 2022

Uh oh!

github-actions Bot commented Dec 19, 2022

Performance Comparison Report 📊

Significant Changes To Duration

Meaningless Changes To Duration

Uh oh!

mananjadhav commented Dec 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviewer Checklist

Screenshots/Videos

Uh oh!

mananjadhav commented Dec 19, 2022

Uh oh!

techievivek commented Dec 20, 2022

Uh oh!

OSBotify commented Dec 20, 2022

Uh oh!

OSBotify commented Dec 21, 2022

Uh oh!

Reviewers

Assignees

Labels

grgia commented Dec 14, 2022 •

edited

Loading

grgia commented Dec 14, 2022 •

edited

Loading

grgia commented Dec 14, 2022 •

edited

Loading

grgia commented Dec 14, 2022 •

edited

Loading

techievivek commented Dec 19, 2022 •

edited

Loading

mananjadhav commented Dec 19, 2022 •

edited

Loading