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

[Due for payment 2025-02-18] [$250] Incorrect border color on subscript avatar in "Choose recipient" page #56015

Closed
1 of 8 tasks
m-natarajan opened this issue Jan 30, 2025 · 34 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 Jan 30, 2025

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.91-2
Reproducible in staging?: Y
Reproducible in production?: Y
If this was caught on HybridApp, is this reproducible on New Expensify Standalone?:
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 (hyperlinked to channel name): expensify_bugs

Action Performed:

  1. Navigate to create expense from Green fab
  2. Enter any amount and click Next
  3. In choose recipient page observe the subscript avatar

Expected Result:

The border color should be lighter to match the background.

Actual Result:

The border color is not matching with the background.

Workaround:

Unknown

Platforms:

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

  • Android: Standalone
  • Android: HybridApp
  • Android: mWeb Chrome
  • iOS: Standalone
  • iOS: HybridApp
  • iOS: mWeb Safari
  • MacOS: Chrome / Safari
  • MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

Image

Image

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~021884807153816511629
  • Upwork Job ID: 1884807153816511629
  • Last Price Increase: 2025-02-03
Issue OwnerCurrent Issue Owner: @VictoriaExpensify
@m-natarajan m-natarajan added Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 labels Jan 30, 2025
Copy link

melvin-bot bot commented Jan 30, 2025

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

@nkdengineer
Copy link
Contributor

Proposal

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

Incorrect border color on subscript avatar

What is the root cause of that problem?

This bug can only be reproduced when hovering into an item and hovering out.

We are defining the set hover logic here

bind: {
onMouseEnter: () => setHovered(true),
onMouseLeave: () => setHovered(false),
},

And use it as props in BaseListItem



https://github.com/Expensify/App/blob/b38c287ac4e462e5c6db786d9443069fac926f04/src/components/SelectionList/BaseListItem.tsx#L77


But onMouseLeave is not a valid props


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

We should use props onHoverIn and onHoverOut

       bind: {
            onHoverIn: () => setHovered(true),
            onHoverOut: () => setHovered(false),
        },

What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?

None

What alternative solutions did you explore? (Optional)

Reminder: Please use plain English, be brief and avoid jargon. Feel free to use images, charts or pseudo-code if necessary. Do not post large multi-line diffs or write walls of text. Do not create PRs unless you have been hired for this job.

@VictoriaExpensify VictoriaExpensify added the External Added to denote the issue can be worked on by a contributor label Jan 30, 2025
Copy link

melvin-bot bot commented Jan 30, 2025

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

@melvin-bot melvin-bot bot changed the title Incorrect border color on subscript avatar in "Choose recipient" page [$250] Incorrect border color on subscript avatar in "Choose recipient" page Jan 30, 2025
@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Jan 30, 2025
Copy link

melvin-bot bot commented Jan 30, 2025

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

@VictoriaExpensify VictoriaExpensify changed the title [$250] Incorrect border color on subscript avatar in "Choose recipient" page [$125] Incorrect border color on subscript avatar in "Choose recipient" page Jan 30, 2025
Copy link

melvin-bot bot commented Jan 30, 2025

Upwork job price has been updated to $125

@VictoriaExpensify
Copy link
Contributor

I think this is a very simple fix so I'm reducing the bounty to $125. If I've misunderstood the complexity of this, please let me know and I can reassess.

@bernhardoj
Copy link
Contributor

Proposal

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

The avatar subscript border has an incorrect color after hovering over it.

What is the root cause of that problem?

The color of the border depends on the hovered state.

backgroundColor={hovered && !isFocused ? hoveredBackgroundColor : subscriptAvatarBorderColor}

The hovered state comes from the useHover hook.

const {hovered, bind} = useHover();

The hook works by "connecting" the bind object to the component that we want to attach. In this case, we attach it to the pressable.

<PressableWithFeedback
// eslint-disable-next-line react/jsx-props-no-spreading
{...bind}
ref={pressableRef}

The bind will attach a mouse enter and mouse leave event.

const [hovered, setHovered] = useState(false);
return {
hovered,
bind: {
onMouseEnter: () => setHovered(true),
onMouseLeave: () => setHovered(false),
},

However, the mouse leave event is never triggered, so the hovered state is stuck and the border color never changes back. It's never triggered because the mouse leave event is overridden here.

onMouseLeave={handleMouseLeave}

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

We need to manually call mouse leave event from bind.

onMouseLeave={(e) => {
    handleMouseLeave(e);
    bind.onMouseLeave();
}}

What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?

I think we can test the BaseListItem and assert that the bind.onMouseEnter and bind.onMouseLeave is called correctly.

@bernhardoj
Copy link
Contributor

@VictoriaExpensify I think we can keep the normal price since it's tricky to find the root cause. We need to create a UI test too.

@VictoriaExpensify VictoriaExpensify changed the title [$125] Incorrect border color on subscript avatar in "Choose recipient" page [$250] Incorrect border color on subscript avatar in "Choose recipient" page Jan 30, 2025
Copy link

melvin-bot bot commented Jan 30, 2025

Upwork job price has been updated to $250

@VictoriaExpensify
Copy link
Contributor

Got it. Thanks for clarifying @bernhardoj

@shawnborton
Copy link
Contributor

Hmm @VictoriaExpensify personally I think we should make this $125, it's going to be a very simple change.

@VictoriaExpensify
Copy link
Contributor

@shawnborton That's what I thought and I changed it to $125, but put it back to $250 after @bernhardoj commented above.

I'm going to put it back to $125 but if you want to give more info about the complexity justifies a $250 bounty, we can review it

@VictoriaExpensify VictoriaExpensify changed the title [$250] Incorrect border color on subscript avatar in "Choose recipient" page [$125] Incorrect border color on subscript avatar in "Choose recipient" page Jan 30, 2025
Copy link

melvin-bot bot commented Jan 30, 2025

Upwork job price has been updated to $125

@shawnborton
Copy link
Contributor

shawnborton commented Jan 30, 2025

Yeah I think if we can't get reasonable proposals at this price point, we can always go up. But I have a feeling it's going to be a super simple change. The border color already changes correctly on hover, so there is nothing we need to do there. We just need to adjust it to be the correct color in this particular instance.

@bernhardoj
Copy link
Contributor

We just need to adjust it to be the correct color in this particular instance.

The color defined is already correct. It's the hover functionality that is currently broken and since the hover functionality is attached to BaseListItem, all list is affected (for example search modal list).

@shawnborton
Copy link
Contributor

The hover styles look fine to me, it's when the row is not hovered that the color seems wrong.

@jjcoffee
Copy link
Contributor

Yeah it looks like the issue is only reproducible when the list item is hovered over first and then unhovered.

chrome-hover-2025-01-30_15.58.53.mp4

@jjcoffee
Copy link
Contributor

I think @bernhardoj's proposal has the right RCA, as it's the onMouseLeave callback not being called that's the real issue. I also think since this isn't a simple style tweak that the issue's price should be $250, especially since we'll want to add a unit test for this. cc @VictoriaExpensify

🎀👀🎀 C+ reviewed

Copy link

melvin-bot bot commented Feb 3, 2025

Upwork job price has been updated to $250

@bernhardoj
Copy link
Contributor

@VictoriaExpensify thanks!

PR is ready

cc: @jjcoffee

@melvin-bot melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Weekly KSv2 labels Feb 11, 2025
@melvin-bot melvin-bot bot changed the title [$250] Incorrect border color on subscript avatar in "Choose recipient" page [Due for payment 2025-02-18] [$250] Incorrect border color on subscript avatar in "Choose recipient" page Feb 11, 2025
@melvin-bot melvin-bot bot removed the Reviewing Has a PR in review label Feb 11, 2025
Copy link

melvin-bot bot commented Feb 11, 2025

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

Copy link

melvin-bot bot commented Feb 11, 2025

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

If no regressions arise, payment will be issued on 2025-02-18. 🎊

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

  • @jjcoffee requires payment through NewDot Manual Requests
  • @bernhardoj requires payment through NewDot Manual Requests

Copy link

melvin-bot bot commented Feb 11, 2025

@jjcoffee @VictoriaExpensify @jjcoffee The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed. Please copy/paste the BugZero Checklist from here into a new comment on this GH and complete it. If you have the K2 extension, you can simply click: [this button]

This comment has been minimized.

@melvin-bot melvin-bot bot added Daily KSv2 Overdue and removed Weekly KSv2 labels Feb 17, 2025
@VictoriaExpensify
Copy link
Contributor

Payment summary:
Contributor: @jjcoffee owed $250 via NewDot

@bernhardoj can you please complete the checklist and I'll get your payment organised 🙌

@melvin-bot melvin-bot bot removed the Overdue label Feb 17, 2025
@bernhardoj
Copy link
Contributor

@jjcoffee is the C+ here.

Requested in ND.

@jjcoffee
Copy link
Contributor

BugZero Checklist:

  • [Contributor] Classify the bug:
Bug classification

Source of bug:

  • 1a. Result of the original design (eg. a case wasn't considered)
  • 1b. Mistake during implementation
  • 1c. Backend bug
  • 1z. Other:

Where bug was reported:

  • 2a. Reported on production (eg. bug slipped through the normal regression and PR testing process on staging)
  • 2b. Reported on staging (eg. found during regression or PR testing)
  • 2d. Reported on a PR
  • 2z. Other:

Who reported the bug:

  • 3a. Expensify user
  • 3b. Expensify employee
  • 3c. Contributor
  • 3d. QA
  • 3z. Other:
  • [Contributor] 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/42823/files#r1959255192

  • [Contributor] If the regression was CRITICAL (e.g. interrupts a core flow) A discussion in #expensify-open-source 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: N/A

  • [Contributor] If it was decided to create a regression test for the bug, please propose the regression test steps using the template below to ensure the same bug will not reach production again.

  • [BugZero Assignee] Create a GH issue for creating/updating the regression test once above steps have been agreed upon.

    Link to issue:

Regression Test Proposal

Precondition:

  • Have at least 1 workspace
  • Desktop & Web only

Test:

  1. Open FAB > Create expense
  2. Enter any amount and go next to the participants
  3. Hover over any workspace participant that has a subscript avatar
  4. Verify the subscript avatar border colour changes to match the background
  5. Hover away from the participant
  6. Verify the subscript avatar border colour returns to its previous state

Do we agree 👍 or 👎

Copy link

melvin-bot bot commented Feb 18, 2025

Payment Summary

Upwork Job

BugZero Checklist (@VictoriaExpensify)

  • I have verified the correct assignees and roles are listed above and updated the neccesary manual offers
  • I have verified that there are no duplicate or incorrect contracts on Upwork for this job (https://www.upwork.com/ab/applicants/1884807153816511629/hired)
  • I have paid out the Upwork contracts or cancelled the ones that are incorrect
  • I have verified the payment summary above is correct

@luacmartins
Copy link
Contributor

I think this is an edge case and a pretty small/harmless bug. I don't think we need to add a specific test case for this.

@VictoriaExpensify
Copy link
Contributor

Oh sorry @bernhardoj! Thanks for the input @luacmartins and the checklist @jjcoffee.

Here's the updated payment summary:
C: @bernhardoj owed $250 via NewDot
C+: @jjcoffee owed $250 via NewDot

@garrettmknight
Copy link
Contributor

$250 approved for @jjcoffee

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
Status: Done
Development

No branches or pull requests

8 participants