Fix: The pressed state of menu items uses a very dark BG color #47036

rayane-djouah · 2024-08-08T07:42:12Z

Details

Fixed Issues

$ #46928
PROPOSAL: #46928 (comment)

Tests

Go to Settings > Profile.
Long-press on menu items.
Verify that the pressed state has a lighter background color compared to the current one.
Click on the FAB.
Long-press on menu items.
Verify that the pressed state has a lighter background color compared to the current one.

Verify that no errors appear in the JS console

Offline tests

N/A

QA Steps

Same as tests

Verify that no errors appear in the JS console

PR Author Checklist

Screenshots/Videos

Android: Native

Screen.Recording.2024-08-18.at.7.11.47.PM.mov

Android: mWeb Chrome

Screen.Recording.2024-08-18.at.7.10.08.PM.mov

iOS: Native

Simulator.Screen.Recording.-.iPhone.15.Pro.Max.-.2024-08-18.at.19.09.53.mp4

iOS: mWeb Safari

Simulator.Screen.Recording.-.iPhone.15.Pro.Max.-.2024-08-18.at.19.07.29.mp4

MacOS: Chrome / Safari

Screen.Recording.2024-08-18.at.6.57.26.PM.mov

MacOS: Desktop

Screen.Recording.2024-08-18.at.7.08.22.PM.mov

melvin-bot · 2024-08-08T07:42:18Z

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

rayane-djouah · 2024-08-08T07:55:53Z

@Expensify/design Could you please confirm which custom BG color we need to use for the pressed state of the menu item?

Current colors on staging:

The current hover state color on staging (theme.hoverComponentBG):

The current active state color on staging (theme.border):

The current pressed color one on staging (theme.buttonPressedBG):

My suggestions for the color of the pressed state (I've already applied a 80% opacity to the pressed state):

Here is the result of this PR (theme.buttonHoveredBG) (A little darker than the active state) (The same pattern with the "invalidate" button mentioned in the issue OP):

Here is the result with theme.border (The same color with the active state):

Here is the result with theme.hoverComponentBG (The same color with the hover state):

shawnborton · 2024-08-08T11:06:20Z

What do we do for our standard buttons? My idea is to just use the same behavior we use for our standard buttons. I tried using a button in Settings > Troubleshoot and it feels like it doesn't get a pressed BG.

rayane-djouah · 2024-08-08T11:24:18Z

@shawnborton - For buttons in Settings > Troubleshoot we use theme.buttonDefaultBG for default state and theme. buttonHoveredBG for hover and pressed states.

One thing to consider is that the menu item has a custom active state color but the standard buttons have not. this is why we use a lighter focus color for the hover state of the menu item compared to the hover state of standard buttons.

Do we want to not use a custom color for the menu item pressed state (keep the hover color) and only apply 80% opacity for the pressed state?

shawnborton · 2024-08-08T11:27:28Z

I'd be curious to see a version where it uses just our colors.border for the active state (the same thing we use for when the rows are in LHN navs for things like Settings or the Workspace editor), as well as where it uses the same active BG as buttons. Can we get a side-by-side comparison of those two?

rayane-djouah · 2024-08-08T11:42:39Z

@shawnborton - Here are videos for both cases:

Using theme.border for the active state:

Screen.Recording.2024-08-08.at.12.38.05.PM.mov

Using theme. buttonHoveredBG for the active state:

Screen.Recording.2024-08-08.at.12.40.19.PM.mov

shawnborton · 2024-08-08T11:45:51Z

I like the first one better, but I generally like the idea of just being consistent with buttons which means me lean towards the second one.

Do we know how the super dark style was ever introduced in the first place though? It feels so strange and like it was recently added. I'm mostly just curious for my own sake.

dannymcclain · 2024-08-08T14:30:21Z

I generally like the idea of just being consistent with buttons which means me lean towards the second one.

I think I agree here. Honestly both are better than the super dark thing we have going on now so I'm not against either, but I like the idea of leaning into patterns we already have established for buttons.

rayane-djouah · 2024-08-08T16:34:46Z

We're not using a custom pressed BG color for selection list items and we're only using a reduced opacity. So I think the first option in #47036 (comment) is more consistent?

Screen.Recording.2024-08-08.at.5.31.47.PM.mov

shawnborton · 2024-08-08T17:04:06Z

Oh interesting... that does seem to make sense to. I guess you could ague either one is consistent with something. Or we make list items consistent with buttons. Let's see if the design team has any strong feelings.

dannymcclain · 2024-08-08T17:26:32Z

No strong feelings from me! I think either works—do you have a strong preference for one vs. the other?

shawnborton · 2024-08-08T17:33:26Z

I think I lean towards 2 so we start establishing a consistent BG color to use for press?

dubielzyk-expensify · 2024-08-09T01:47:50Z

Sounds good to me as well

eh2077 · 2024-08-12T13:36:52Z

@rayane-djouah Does the PR need to be updated to meet design team's decisions?

rayane-djouah · 2024-08-13T12:40:55Z

Yes, will update today

…very-dark-BG-color

…MenuItem

shawnborton · 2024-08-16T13:28:20Z

What's the latest on this one?

rayane-djouah · 2024-08-18T18:21:05Z

PR ready for review

shawnborton · 2024-08-19T14:55:08Z

Hmm when I test this, it seems like the BG color changes quickly and then it quickly changes again even though I had my mouse pressed the entire time. Any ideas?

CleanShot.2024-08-19.at.10.54.16.mp4

shawnborton · 2024-08-23T15:16:42Z

How can we fix that? Can we make them use the same animation or something so they change at the same time?

…very-dark-BG-color

rayane-djouah · 2024-08-27T23:42:48Z

How can we fix that? Can we make them use the same animation or something so they change at the same time?

Hmm, they technically are changing at the same time, so I'm not sure why this is happening. @eh2077, any ideas?

…ms-uses-a-very-dark-BG-color

eh2077 · 2024-08-29T01:13:21Z

How can we fix that? Can we make them use the same animation or something so they change at the same time?

Hmm, they technically are changing at the same time, so I'm not sure why this is happening. @eh2077, any ideas?

Thanks for the ping. The issue seems subtle. I'll have to take some time to investigate it and get back to here later.

eh2077 · 2024-09-02T16:12:11Z

@rayane-djouah If the opacity only needs to be applied to the background color, then maybe we can try rgb(). If not, are you able to reproduce the issue with a minimal demo? So we can understand the issue better

shawnborton · 2024-09-05T12:46:44Z

What's the latest on this one? Let's try to get this one over the finish line!

rayane-djouah · 2024-09-08T23:05:48Z

Sorry for the delay; I was focusing on critical / high-priority issues.

@shawnborton - I'm having trouble understanding the expected result from this bug report. Could you please clarify a bit more or share a mockup/prototype video of the expected result? That would be very helpful.

shawnborton · 2024-09-09T05:32:14Z

Yeah - basically it feels like the background color of the menuItem changes to the darker color quickly, and then we see the animated change in opacity. So you get a bit of like a double effect where the color changes and then the opacity changes.

It would feel smoother if that could all happen at once so you don't get that slight flashing effect. Let me know if that is helpful!

…very-dark-BG-color

rayane-djouah · 2024-09-15T12:15:20Z

Updated!

Screen.Recording.2024-09-15.at.1.14.34.PM.mov

cc @shawnborton

shawnborton · 2024-09-16T05:17:56Z

That seems pretty good to me!

github-actions · 2024-09-16T05:53:47Z

🧪🧪 Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! 🧪🧪

Android 🤖	iOS 🍎
https://ad-hoc-expensify-cash.s3.amazonaws.com/android/47036/index.html	https://ad-hoc-expensify-cash.s3.amazonaws.com/ios/47036/index.html

Desktop 💻	Web 🕸️
https://ad-hoc-expensify-cash.s3.amazonaws.com/desktop/47036/NewExpensify.dmg	https://47036.pr-testing.expensify.com

👀 View the workflow run that generated this build 👀

shawnborton · 2024-09-16T05:56:00Z

Okay let's get this into final review + merge @eh2077

eh2077 · 2024-09-16T13:39:47Z

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: mWeb Chrome
- iOS: Native
- iOS: mWeb 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 either coming verbatim from figma or has been approved by marketing (in order to get marketing approval, ask the Bug Zero team member to add the Waiting for copy label to the issue)
- 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(theme.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 the PR modifies a component or page that can be accessed by a direct deeplink, I verified that the code functions as expected when the deeplink is used - from a logged in and logged out account.
If the PR modifies the UI (e.g. new buttons, new UI components, changing the padding/spacing/sizing, moving components, etc) or modifies the form input styles:
- I verified that all the inputs inside a form are aligned with each other.
- I added Design label and/or tagged @Expensify/design so the design team can review the changes.
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

Android: Native

0-android.mp4

Android: mWeb Chrome

0-mobile-chrome.mp4

iOS: Native

0-ios.mp4

iOS: mWeb Safari

0-ios.mp4

MacOS: Chrome / Safari

0-web.mp4

MacOS: Desktop

0-desktop.mp4

eh2077

LGTM! Just a minor comment

src/components/PressableWithSecondaryInteraction/types.ts

eh2077

LGTM!

OSBotify · 2024-09-16T18:45:39Z

✋ 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 · 2024-09-16T20:31:27Z

🚀 Deployed to staging by https://github.com/arosiclair in version: 9.0.36-1 🚀

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

OSBotify · 2024-09-17T20:31:00Z

🚀 Deployed to production by https://github.com/grgia in version: 9.0.36-2 🚀

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

Fix: The pressed state of option menu items uses a very dark BG color

a12bf41

rayane-djouah requested review from a team as code owners August 8, 2024 07:42

melvin-bot bot requested review from eh2077 and removed request for a team August 8, 2024 07:42

rayane-djouah marked this pull request as draft August 13, 2024 12:46

rayane-djouah added 3 commits August 13, 2024 23:43

Merge branch 'main' into Fix--The-pressed-state-of-menu-items-uses-a-…

2228b78

…very-dark-BG-color

Merge branch 'main' into Fix--The-pressed-state-of-menu-items-uses-a-…

6e1480a

…very-dark-BG-color

Update button pressed state background color style in BaseMiniContext…

33ed8e9

…MenuItem

rayane-djouah marked this pull request as ready for review August 18, 2024 18:20

mountiny added the Ready To Build label Aug 19, 2024

This comment has been minimized.

Sign in to view

Merge branch 'main' into Fix--The-pressed-state-of-menu-items-uses-a-…

38d6ecb

…very-dark-BG-color

rayane-djouah added 2 commits August 28, 2024 17:34

Merge branch 'Expensify:main' into Fix--The-pressed-state-of-menu-ite…

b876e1c

…ms-uses-a-very-dark-BG-color

Fix Lint error

95aef26

rayane-djouah added 2 commits September 15, 2024 12:42

Merge branch 'main' into Fix--The-pressed-state-of-menu-items-uses-a-…

e5e4891

…very-dark-BG-color

Fix opacity animation duration

1db6abb

eh2077 reviewed Sep 16, 2024

View reviewed changes

src/components/PressableWithSecondaryInteraction/types.ts Outdated Show resolved Hide resolved

Remove unused property

13d82c3

rayane-djouah requested a review from eh2077 September 16, 2024 13:56

eh2077 approved these changes Sep 16, 2024

View reviewed changes

melvin-bot bot requested a review from arosiclair September 16, 2024 14:33

arosiclair approved these changes Sep 16, 2024

View reviewed changes

arosiclair merged commit 0cd67be into Expensify:main Sep 16, 2024
16 of 17 checks passed

rayane-djouah deleted the Fix--The-pressed-state-of-menu-items-uses-a-very-dark-BG-color branch September 16, 2024 18:49

github-actions bot mentioned this pull request Sep 16, 2024

Deploy Checklist: New Expensify 2024-09-16 #49287

Closed

38 tasks

rayane-djouah mentioned this pull request Sep 17, 2024

[HOLD for Payment 2024-09-24] [$125] The pressed state of option rows/menu items uses a very dark BG color #46928

Closed

6 tasks

Fix: The pressed state of menu items uses a very dark BG color #47036

Fix: The pressed state of menu items uses a very dark BG color #47036

Conversation

rayane-djouah commented Aug 8, 2024 • edited Loading

Details

Fixed Issues

Tests

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

melvin-bot bot commented Aug 8, 2024

rayane-djouah commented Aug 8, 2024 • edited Loading

Current colors on staging:

My suggestions for the color of the pressed state (I've already applied a 80% opacity to the pressed state):

shawnborton commented Aug 8, 2024

rayane-djouah commented Aug 8, 2024 • edited Loading

shawnborton commented Aug 8, 2024

rayane-djouah commented Aug 8, 2024

shawnborton commented Aug 8, 2024

dannymcclain commented Aug 8, 2024

rayane-djouah commented Aug 8, 2024

shawnborton commented Aug 8, 2024

dannymcclain commented Aug 8, 2024

shawnborton commented Aug 8, 2024

dubielzyk-expensify commented Aug 9, 2024

eh2077 commented Aug 12, 2024

rayane-djouah commented Aug 13, 2024

shawnborton commented Aug 16, 2024

rayane-djouah commented Aug 18, 2024

This comment has been minimized.

shawnborton commented Aug 19, 2024

shawnborton commented Aug 23, 2024

rayane-djouah commented Aug 27, 2024

eh2077 commented Aug 29, 2024

eh2077 commented Sep 2, 2024

shawnborton commented Sep 5, 2024

rayane-djouah commented Sep 8, 2024 • edited Loading

shawnborton commented Sep 9, 2024

rayane-djouah commented Sep 15, 2024

shawnborton commented Sep 16, 2024

github-actions bot commented Sep 16, 2024

shawnborton commented Sep 16, 2024

eh2077 commented Sep 16, 2024 • edited Loading

Reviewer Checklist

Screenshots/Videos

eh2077 left a comment

Choose a reason for hiding this comment

eh2077 left a comment

Choose a reason for hiding this comment

OSBotify commented Sep 16, 2024

OSBotify commented Sep 16, 2024

OSBotify commented Sep 17, 2024

rayane-djouah commented Aug 8, 2024 •

edited

Loading

rayane-djouah commented Aug 8, 2024 •

edited

Loading

rayane-djouah commented Aug 8, 2024 •

edited

Loading

rayane-djouah commented Sep 8, 2024 •

edited

Loading

eh2077 commented Sep 16, 2024 •

edited

Loading