[WAITING ON SOBIT - CHECKLIST] [$250] Selected row in onboarding modal should not get :hover style

[m-natarajan]

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: 1.4.86-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/p1718816345646859

Action Performed:

1. Sign up for a new account
2. In the onboarding modal, select an option
3. Hover over the selected option

Expected Result:

Selected row should not get a hover style

Actual Result:

Selected row changes BG color on hover

Workaround:

unknown

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

CleanShot.2024-06-28.at.16.12.11.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01ffaa9b43dacb7a8a
• Upwork Job ID: 1806378179434309152
• Last Price Increase: 2024-07-04
• Automatic offers:
• truph01 | Contributor | 103133129

Issue Owner

Current Issue Owner: @mallenexpensify

[melvin-bot]

Triggered auto assignment to @mallenexpensify (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.

[truph01]

Proposal

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

• Selected row in onboarding modal should not get :hover style

What is the root cause of that problem?

• We always display the hover style if the item is hovered without considering if it is selected or not:
  

  App/src/components/MenuItem.tsx

  Line 517 in 937c418

  isHovered && interactive && !pressed && styles.hoveredComponentBG,

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

• We should remove:

App/src/components/MenuItem.tsx

Line 517 in 937c418

isHovered && interactive && !pressed && styles.hoveredComponentBG,

which used to fix issue.

• Then fix that issue by adding

                        hoverAndPressStyle={[styles.hoveredComponentBG]}

to:

App/src/components/ReportActionItem/MoneyRequestView.tsx

Line 467 in 937c418

<MenuItemWithTopDescription

• We need to fix other similar positions that also use MenuItemWithTopDescription. Or we just need to update:
  

  App/src/components/MenuItemWithTopDescription.tsx

  Line 9 in 937c418

  <MenuItem

  
  to:

   <MenuItem hoverAndPressStyle={props.hoverAndPressStyle ?? styles.hoveredComponentBG ...

• Then we update this:
  

  App/src/pages/OnboardingPurpose/BaseOnboardingPurpose.tsx

  Lines 99 to 100 in 29e4c54

  	wrapperStyle: [styles.purposeMenuItem, isSelected && styles.purposeMenuItemSelected],
  	hoverAndPressStyle: [styles.purposeMenuItemSelected],

  
  to:

            focused: isSelected,
            style: [styles.purposeMenuItem],
            hoverAndPressStyle: [styles.hoveredComponentBG]

to fix our main issue.

What alternative solutions did you explore? (Optional)

• We can update:
  

  App/src/components/MenuItem.tsx

  Line 517 in 937c418

  isHovered && interactive && !pressed && styles.hoveredComponentBG,

  
  to:

                                        isHovered && interactive && !focused && !pressed && styles.hoveredComponentBG,

then add:

    focused: isSelected

to:

App/src/pages/OnboardingPurpose/BaseOnboardingPurpose.tsx

Line 91 in 29e4c54

return {

[neonbhai]

Proposal

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

Onboarding modal uses incorrect row hover style when hovering over the options

What is the root cause of that problem?

There are two problems:

• The hoverStyle is being passed incorrectly here

• The MenuItem styles here applied in the wrong order.

  Current order applies hover style after the active button style giving us this behavior:

  Video (hover applied after active style)

  Screen.Recording.2024-06-20.at.12.14.03.PM.mp4

Additionally:

This order effectively prevents us from seeing the pressed style for menuItem as defined here:

App/src/styles/utils/index.ts

Lines 1320 to 1321 in 7561439

	case CONST.BUTTON_STATES.PRESSED:
	return {backgroundColor: theme.buttonPressedBG};

Which gives us this tactile feedback when pressing:

Video

Screen.Recording.2024-06-20.at.12.17.43.PM.mov

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

• change this line to:

  hoverAndPressStyle: [styles.hoveredComponentBG],

• Move this style:
  

  App/src/components/MenuItem.tsx

  Line 513 in 7561439

  StyleUtils.getButtonBackgroundColorStyle(getButtonState(focused || isHovered, pressed, success, disabled, interactive), true),

  
  after this line:
  

  App/src/components/MenuItem.tsx

  Line 515 in 7561439

  !focused && (isHovered || pressed) && hoverAndPressStyle,

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

Alternatively

If we don't want feedback on active press we can add a check for !isMenuItem here

[truph01]

Updated proposal

[melvin-bot]

@shawnborton, @mallenexpensify Eep! 4 days overdue now. Issues have feelings too...

[shawnborton]

@mallenexpensify can you please review this when you get a moment? Thanks!

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~01ffaa9b43dacb7a8a

[melvin-bot]

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

[mallenexpensify]

@sobitneupane , 👀 on the above proposals plz? Please keep in mind at @truph01 's proposal had been updated many times

[truph01]

@mallenexpensify Yeah, my last update time was 3:04 PM, and then I commented "Proposal Updated" at 3:10 PM as mentioned in the contributor guides.

[mallenexpensify]

@truph01 , yes, please continue to post 'proposal updated' when you do. I was referencing that updates were made over a handful of hours which might affect consideration of other proposals if they were submitted within that time.

[sobitneupane]

@shawnborton Looks like the issue is not reproducible any longer.

Screen.Recording.2024-06-28.at.13.37.59.mov

[shawnborton]

Interesting, if that's the case, we can just close this. Let me test quickly as well...

[shawnborton]

Indeed it looks fixed. But let me ask a follow up question for the @Expensify/design team - should we make it so that the row that has the :active/:selected state should not get a hover BG? I think yes? But I don't feel too strongly. I think our existing patterns in the app make it so that anything that has a selected state doesn't get a hover BG color (like LHN items for instance).

[dannymcclain]

Yes 100% agree on that point Shawn. It's weird for the selected item to go from heavily emphasized to not-as-emphasized on hover.

[shawnborton]

Okay cool, I'll go ahead and modify the title + body, and maybe we can take a new round of proposals here?

[truph01]

Proposal updated

• I updated alternative solution.

[truph01]

Proposal updated

• I updated main solution.

[sobitneupane]

Thanks for the update @truph01

Your alternative solution looks good to me but I believe we would want that behavior throughout the app not only in onboarding modal. Maybe we can use existing props in MenuItem instead of passing isSelected prop.

[melvin-bot]

Reviewing label has been removed, please complete the "BugZero Checklist".

[melvin-bot]

The solution for this issue has been 🚀 deployed to production 🚀 in version 9.0.12-0 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• Feat: Selected row in onboarding modal should not get :hover style #45499

If no regressions arise, payment will be issued on 2024-08-02. 🎊

For reference, here are some details about the assignees on this issue:

• @sobitneupane requires payment through NewDot Manual Requests
• @truph01 requires payment automatic offer (Contributor)

[melvin-bot]

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

• [@sobitneupane] The PR that introduced the bug has been identified. Link to the PR:
• [@sobitneupane] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
• [@sobitneupane] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:
• [@sobitneupane] Determine if we should create a regression test for this bug.
• [@sobitneupane] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
• [@mallenexpensify] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[melvin-bot]

Triggered auto assignment to @jliexpensify (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.

[mallenexpensify]

@jliexpensify I'm off til Monday afternoon, can you help with payment here? Thx

[jliexpensify]

Payment Summary

• C: @truph01 $250 (Upwork job)
• C+: @sobitneupane (ND payment)

Waiting on checklist

[jliexpensify]

Paid and job closed. @sobitneupane bump for checklist!

[sobitneupane]

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

• [@sobitneupane] The PR that introduced the bug has been identified. Link to the PR:

#43987

• [@sobitneupane] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:

https://github.com/Expensify/App/pull/43987/files#r1703875086

• [@sobitneupane] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:

I don't think any new update is required.

• [@sobitneupane] Determine if we should create a regression test for this bug.

Yes.

• [@sobitneupane] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.

#44055 (comment)

[sobitneupane]

Regression Test Proposal

1. Sign up for a new account
2. In the onboarding modal, select an option
3. Hover over the selected option
4. Verify that the selected option does not get hover style.

Do we agree 👍 or 👎

[melvin-bot]

@shawnborton, @mallenexpensify, @sobitneupane, @jliexpensify, @marcaaron, @truph01 Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[marcaaron]

LGTM @sobitneupane

[mallenexpensify]

Test Case GH, thanks @sobitneupane
https://github.com/Expensify/Expensify/issues/418347

[garrettmknight]

$250 approved for @sobitneupane based on this summary.