Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
1 of 6 tasks
m-natarajan opened this issue Aug 6, 2024 · 21 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor

Comments

@m-natarajan
Copy link

m-natarajan commented Aug 6, 2024

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
  • 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
@m-natarajan m-natarajan added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Aug 6, 2024
Copy link

melvin-bot bot commented Aug 6, 2024

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
Copy link
Contributor

rayane-djouah commented Aug 6, 2024

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:

StyleUtils.getButtonBackgroundColorStyle(getButtonState(focused || isHovered, pressed, success, disabled, interactive), true),
...(Array.isArray(wrapperStyle) ? wrapperStyle : [wrapperStyle]),

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

return {backgroundColor: theme.buttonPressedBG};

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 :

<PressableWithSecondaryInteraction
onPress={shouldCheckActionAllowedOnPress ? Session.checkIfActionIsAllowed(onPressAction, isAnonymousAction) : onPressAction}
onPressIn={() => shouldBlockSelection && shouldUseNarrowLayout && DeviceCapabilities.canUseTouchScreen() && ControlSelection.block()}
onPressOut={ControlSelection.unblock}
onSecondaryInteraction={onSecondaryInteraction}
wrapperStyle={outerWrapperStyle}
style={({pressed}) =>
[
containerStyle,
combinedStyle,
!interactive && styles.cursorDefault,
!shouldRemoveBackground &&
StyleUtils.getButtonBackgroundColorStyle(getButtonState(focused || isHovered, pressed, success, disabled, interactive), true),
...(Array.isArray(wrapperStyle) ? wrapperStyle : [wrapperStyle]),
!focused && (isHovered || pressed) && hoverAndPressStyle,
shouldGreyOutWhenDisabled && disabled && styles.buttonOpacityDisabled,
isHovered && interactive && !focused && !pressed && !shouldRemoveBackground && styles.hoveredComponentBG,
] as StyleProp<ViewStyle>
}
disabledStyle={shouldUseDefaultCursorWhenDisabled && [styles.cursorDefault]}
disabled={disabled || isExecuting}
ref={ref}
role={CONST.ROLE.MENUITEM}
accessibilityLabel={title ? title.toString() : ''}
accessible
onFocus={onFocus}
>

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

return {backgroundColor: theme.buttonPressedBG};

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:

Screen.Recording.2024-08-06.at.10.32.08.PM.mov

What alternative solutions did you explore? (Optional)

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

@twisterdotcom twisterdotcom added the External Added to denote the issue can be worked on by a contributor label Aug 6, 2024
Copy link

melvin-bot bot commented Aug 6, 2024

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

@melvin-bot melvin-bot bot changed the title The pressed state of option rows/menu items uses a very dark BG color [$250] The pressed state of option rows/menu items uses a very dark BG color Aug 6, 2024
@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Aug 6, 2024
Copy link

melvin-bot bot commented Aug 6, 2024

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

@twisterdotcom twisterdotcom changed the title [$250] The pressed state of option rows/menu items uses a very dark BG color [$125] The pressed state of option rows/menu items uses a very dark BG color Aug 6, 2024
Copy link

melvin-bot bot commented Aug 6, 2024

Upwork job price has been updated to $125

@eh2077
Copy link
Contributor

eh2077 commented Aug 7, 2024

@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

Copy link

melvin-bot bot commented Aug 7, 2024

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

@dominictb
Copy link
Contributor

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.

Screen.Recording.2024-08-07.at.11.51.07.AM.mov

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

return {backgroundColor: theme.buttonPressedBG};
, 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

  1. 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)

  1. 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
Copy link
Contributor

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
Copy link

zinzaducnm commented Aug 7, 2024

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

expensify-app-46928-demo.mov

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.

Copy link

melvin-bot bot commented Aug 7, 2024

📣 @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 melvin-bot bot removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Aug 7, 2024
Copy link

melvin-bot bot commented Aug 7, 2024

📣 @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

Copy link

melvin-bot bot commented Aug 7, 2024

📣 @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 📖

Copy link

melvin-bot bot commented Aug 8, 2024

⚠️ 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
Copy link

zinzaducnm commented Aug 8, 2024

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

Copy link

melvin-bot bot commented Aug 8, 2024

⚠️ 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
Copy link
Contributor

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 melvin-bot bot added Weekly KSv2 and removed Daily KSv2 labels Aug 8, 2024
@melvin-bot melvin-bot bot added Weekly KSv2 and removed Weekly KSv2 labels Aug 18, 2024
@melvin-bot melvin-bot bot added Monthly KSv2 and removed Weekly KSv2 labels Sep 11, 2024
Copy link

melvin-bot bot commented Sep 11, 2024

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
Copy link
Contributor

I'm still working on addressing PR review comments.

@rayane-djouah
Copy link
Contributor

PR on staging

@rayane-djouah
Copy link
Contributor

Note

The production deploy automation failed: This should be on [HOLD for Payment 2024-09-24] according to #49287 production deploy checklist, confirmed in #47036 (comment)

cc @twisterdotcom

@twisterdotcom twisterdotcom changed the title [$125] The pressed state of option rows/menu items uses a very dark BG color [HOLD for Payment 2024-09-24] [$125] The pressed state of option rows/menu items uses a very dark BG color Sep 18, 2024
@twisterdotcom twisterdotcom added Awaiting Payment Auto-added when associated PR is deployed to production and removed Reviewing Has a PR in review labels Sep 18, 2024
@melvin-bot melvin-bot bot added Daily KSv2 and removed Monthly KSv2 labels Sep 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor
Projects
None yet
Development

No branches or pull requests

7 participants