Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
3.35k stars 2.77k forks source link

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

Open m-natarajan opened 1 month ago

m-natarajan commented 1 month ago

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!


Version Number: 9.0.17-0 Reproducible in staging?: y Reproducible in production?: y 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 Expensify/Expensify Issue URL: Issue reported by: @shawnborton Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1722977671278059

Action Performed:

  1. Go to workspace settings
  2. Go to more features > Toggle Workflows
  3. Go to workflows
  4. Long press the "Add approval workflow" or any option row

    Expected Result:

    Should be simple opacity reduction, custom BG color

    Actual Result:

    notice that it uses a very dark BG color.

    Workaround:

    visual

    Platforms:

    Which of our officially supported platforms is this issue occurring on?

    • [ ] Android: Native
    • [ ] Android: mWeb Chrome
    • [ ] iOS: Native
    • [ ] iOS: mWeb Safari
    • [x] MacOS: Chrome / Safari
    • [ ] MacOS: Desktop

Screenshots/Videos

Screenshot 2024-08-06 175312

image (13)

CleanShot 2024-08-06 at 16 55 12@2x

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~016a8138980835380a
  • Upwork Job ID: 1820968618804470137
  • Last Price Increase: 2024-08-06
  • Automatic offers:
    • eh2077 | Reviewer | 103434092
    • rayane-djouah | Contributor | 103434093
Issue OwnerCurrent Issue Owner: @eh2077
melvin-bot[bot] commented 1 month ago

Triggered auto assignment to @twisterdotcom (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

rayane-djouah commented 1 month ago

Proposal

Please re-state the problem that we are trying to solve in this issue

The state of our menu item became so dark when it's pressed. We need to just reduce its opacity to 80% when pressed, and use a lighter background color compared to the currently used one.

What is the root cause of that problem?

in MenuItem's PressableWithSecondaryInteraction we use getButtonBackgroundColorStyle style function:

https://github.com/Expensify/App/blob/1147c3b3b4b057e6894bfa9aebd1a910cefc5c7c/src/components/MenuItem.tsx#L545-L546

and getButtonBackgroundColorStyle returns theme.buttonPressedBG background color when the button state is pressed:

https://github.com/Expensify/App/blob/1147c3b3b4b057e6894bfa9aebd1a910cefc5c7c/src/styles/utils/index.ts#L1293

What changes do you think we should make in order to solve the problem?

we should use activeOpacity={0.8} here to reduce its opacity to 80% when pressed :

https://github.com/Expensify/App/blob/1147c3b3b4b057e6894bfa9aebd1a910cefc5c7c/src/components/MenuItem.tsx#L533-L559

and isMenuItem ? {backgroundColor: theme. buttonHoveredBG} : {backgroundColor: theme.buttonPressedBG}; here:

https://github.com/Expensify/App/blob/1147c3b3b4b057e6894bfa9aebd1a910cefc5c7c/src/styles/utils/index.ts#L1293

To make the menu item background color theme.buttonHoveredBG when it's pressed.

We can ajust the color or define meaningful names as necessary.

Result:

https://github.com/user-attachments/assets/cf7d7984-60a1-4cfd-819b-c457ca3083e0

What alternative solutions did you explore? (Optional)

Use the same theme.border style for both the active and pressed states of the menu item.

melvin-bot[bot] commented 1 month ago

Job added to Upwork: https://www.upwork.com/jobs/~016a8138980835380a

melvin-bot[bot] commented 1 month ago

Triggered auto assignment to Contributor-plus team member for initial proposal review - @eh2077 (External)

melvin-bot[bot] commented 1 month ago

Upwork job price has been updated to $125

eh2077 commented 1 month ago

@rayane-djouah 's proposal looks good to me. We should ensure both light and dark modes working as expected - we can check this in PR

πŸŽ€πŸ‘€πŸŽ€ C+ reviewed

melvin-bot[bot] commented 1 month ago

Triggered auto assignment to @arosiclair, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

dominictb commented 1 month ago

Heads up @eh2077 I think both parts of the solutions of the selected proposal are not correct, in the result video you can also see that the hover color of the menu item is wrong, it's lighter, while when pressing it's darker than when hovering.

https://github.com/user-attachments/assets/32b5b45b-00b6-487e-b489-0f7963e3780e

The correct hover and press behavior (that all other buttons are following) is, the hover color will be theme.buttonHoveredBG, and when pressed we'll apply 0.8 opacity on top of that, so the background color when pressing will be lighter than hovering. See video below: https://github.com/user-attachments/assets/cc6297f6-12bf-4400-9566-7f0a18a178d4

Proposal

Please re-state the problem that we are trying to solve in this issue.

Very dark BG color in menu items around the app.

What is the root cause of that problem?

When the menu item is pressed, it will have this style https://github.com/Expensify/App/blob/de894796363ad1bc42fa360c6d7b74f8a26c4b61/src/styles/utils/index.ts#L1293, which is darker than normal hover style of buttons.

And the MenuItem is incorrectly using default activeOpacity as 1 here which is wrong and overrides the correct default pressDimmingValue (0.8) that is existing in PressableWithFeedback here

What changes do you think we should make in order to solve the problem?

  1. Remove the default 1 here

    activeOpacity,

    So PressableWithSecondaryInteraction will use the same correct default of PressableWithFeedback here

  2. As Button will have hover style of buttonDefaultHovered (which uses buttonHoveredBG) here, and the 0.8 opacity when pressed is supposed to be based on that color. We should do same for MenuItem, so we can add here

    hoverStyle={!disabled && styles.buttonDefaultHovered}

    (or just use buttonHoveredBG)

  3. And in here we can update to

    return isMenuItem ? {backgroundColor: theme.buttonHoveredBG} : {backgroundColor: theme.buttonPressedBG};

What alternative solutions did you explore? (Optional)

For 1st part, we can check other usage of PressableWithSecondaryInteraction, if there's any place that requires activeOpacity as 1, set it explicitly as 1 while the default will remain as 0.8

For 2nd part, if we want the hover style of MenuItem to stay the same, we can omit this part. But nevertheless the backgroundColor for part 3 for menu item has to be buttonHoveredBG and not hoverComponentBG as hoverComponentBG is a much lighter color and would not match our color scheme.

rayane-djouah commented 1 month ago

The resulting video in my proposal demonstrates the use of theme.buttonHoveredBG.

hoverComponentBG was a typo here (I've corrected it now):

and isMenuItem ? {backgroundColor: theme.hoverComponentBG} : {backgroundColor: theme.buttonPressedBG};

In my proposal, I mentioned:

We can adjust the color or define meaningful names as necessary.

Because:

Initially, I thought to use the same theme.border for both the active and pressed states of the menu item.

Later, I realized that the OP suggests the same behavior for the "invalidate" button, and it states:

Expected Result: Should be a simple opacity reduction, custom BG color

The "invalidate" button uses theme.buttonHoveredBG when hovered and theme.buttonPressedBG when pressed. The menu item uses theme.border when hovered and theme.buttonPressedBG when pressed.

In the light theme, for example, the colors are:

buttonHoveredBG: colors.productLight500
buttonPressedBG: colors.productLight600

border: colors.productLight400

Because theme.border for the active state of MenuItem is lighter than theme.buttonHoveredBG, I suggested using theme.buttonHoveredBG for the pressed state of MenuItem which is lighter than theme.buttonPressedBG, and considered defining new, meaningful names for these values.

However, we should get confirmation from the design team on which color to choose.

Additionally, I don't think changing the default value of activeOpacity in PressableWithSecondaryInteraction is advisable without checking all usages to ensure that we don't introduce regressions. This is why I suggested the change only in the MenuItem component as we did already for OptionRowLHN here, but we can consider this during PR review.

zinzaducnm commented 1 month ago

Contributor details Your Expensify account email: duc.nm@zinza.com.vn Upwork Profile Link: https://www.upwork.com/freelancers/~01f7a872ff2a3998ea

Proposal

Please re-state the problem we are trying to solve in this issue.

In workspace settings > Toggle workflows, when a user presses in and does not press out on any option row, it uses a very dark background color (productLight600: '#C7BFB3').

What is the root cause of that problem?

The Section list in WorkspaceWorkflowsPage uses ToggleSettingOptionRow. Each ToggleSettingOptionRow has submenu items as MenuItem. The MenuItem feature sets the background color {backgroundColor: theme.buttonPressedBG} when the MenuItem state is pressed. This is the productLight600 color of lightTheme. All screens using MenuItem (e.g., task, country select, modal menu, etc.) share this behavior.

What changes do you think we should make to solve the problem?

Modify the getButtonBackgroundColorStyle function to check if the buttonState is pressed and isMenuItem is true then return the {backgroundColor: theme.border} instead of {backgroundColor: theme.buttonPressedBG}

The code changes as follows.

/**
     * Generate a style for the background color of the button, based on its current state.
     *
     * @param buttonState - One of {'default', 'hovered', 'pressed'}
     * @param isMenuItem - whether this button is apart of a list
     */
    getButtonBackgroundColorStyle: (buttonState: ButtonStateName = CONST.BUTTON_STATES.DEFAULT, isMenuItem = false): ViewStyle => {
        switch (buttonState) {
            case CONST.BUTTON_STATES.PRESSED:
                return isMenuItem ? {backgroundColor: theme.border} : {backgroundColor: theme.buttonPressedBG};
            case CONST.BUTTON_STATES.ACTIVE:
                return isMenuItem ? {backgroundColor: theme.border} : {backgroundColor: theme.buttonHoveredBG};
            case CONST.BUTTON_STATES.DISABLED:
            case CONST.BUTTON_STATES.DEFAULT:
            default:
                return {};
        }
    },

Video illustration after the change

https://github.com/user-attachments/assets/cf7d382f-efe8-44fb-99df-ffdcf5c9c03b

What alternative solutions did you explore? (Optional)

Another approach is to change the MenuItem behavior to use a lighter color in the pressed state. This will affect all screens using MenuItem.

melvin-bot[bot] commented 1 month ago

πŸ“£ @zinzaducnm! πŸ“£ Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork. Please follow these steps:

  1. Make sure you've read and understood the contributing guidelines.
  2. Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
  3. Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
  4. Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details. Screen Shot 2022-11-16 at 4 42 54 PM Format:
    Contributor details
    Your Expensify account email: <REPLACE EMAIL HERE>
    Upwork Profile Link: <REPLACE LINK HERE>
melvin-bot[bot] commented 1 month ago

πŸ“£ @eh2077 πŸŽ‰ An offer has been automatically sent to your Upwork account for the Reviewer role πŸŽ‰ Thanks for contributing to the Expensify app!

Offer link Upwork job

melvin-bot[bot] commented 1 month ago

πŸ“£ @rayane-djouah πŸŽ‰ An offer has been automatically sent to your Upwork account for the Contributor role πŸŽ‰ Thanks for contributing to the Expensify app!

Offer link Upwork job Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review πŸ§‘β€πŸ’» Keep in mind: Code of Conduct | Contributing πŸ“–

melvin-bot[bot] commented 1 month ago

⚠️ Missing/invalid email or upwork profile link. Please make sure you add both your Expensify email and Upwork profile link in the format specified.

zinzaducnm commented 1 month ago

Contributor details Your Expensify account email: duc.nm@zinza.com.vn Upwork Profile Link: https://www.upwork.com/freelancers/~01f7a872ff2a3998ea

melvin-bot[bot] commented 1 month ago

⚠️ Missing/invalid email or upwork profile link. Please make sure you add both your Expensify email and Upwork profile link in the format specified.

dominictb commented 1 month ago

Hi @arosiclair I have concerns above that the selected proposal does not work and will cause inconsistencies. Could we please hold assignment for a bit so @eh2077 can re-review the proposals so we know which one is best to move forward with?

Thanks πŸ‘

melvin-bot[bot] commented 1 week ago

This issue has not been updated in over 15 days. @arosiclair, @twisterdotcom, @rayane-djouah, @eh2077 eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!

rayane-djouah commented 1 week ago

I'm still working on addressing PR review comments.

rayane-djouah commented 6 days ago

PR on staging

rayane-djouah commented 6 days ago

[!NOTE] The production deploy automation failed: This should be on [HOLD for Payment 2024-09-24] according to https://github.com/Expensify/App/issues/49287 production deploy checklist, confirmed in https://github.com/Expensify/App/pull/47036#issuecomment-2356865492

cc @twisterdotcom