[HOLD for BZ checklist] [$2000] mWeb - Manage Members - Blue outline at checkbox @Puneet-here

[kbecciv]

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

---

Action Performed:

1. Go to staging.new.expensify.com
2. Go to settings > workspace
3. Manage members page
4. Select a user now tap on the checkbox of that user

Expected Result:

The checkbox shouldn't get blue outline

Actual Result:

The checkbox gets blue outline

Workaround:

Unknown

Platform:

Where is this issue occurring?

• Mobile Web

Version Number: 1.2.6.0

Reproducible in staging?: Yes

Reproducible in production?: Yes

Email or phone of affected tester (no customers): any

Logs: https://stackoverflow.com/c/expensify/questions/4856

Notes/Photos/Videos: Any additional supporting documentation

Screen.Recording.2022-09-05.at.10.47.19.PM.mov

Expensify/Expensify Issue URL:

Issue reported by: @Puneet-here

Slack conversation: https://expensify.slack.com/archives/C01GTK53T8Q/p1662398489632479

View all open jobs on GitHub

[melvin-bot]

Triggered auto assignment to @laurenreidexpensify (AutoAssignerTriage), see https://stackoverflow.com/c/expensify/questions/4749 for more details.

[varshamb]

Remove styles.borderColorFocus
By removing this, we will have same behaviour for clicking checkbox and clicking on member name.

App/src/components/Checkbox.js

Line 107 in 6bd5304

this.state.isFocused && styles.borderColorFocus,

Screen.Recording.2022-09-26.at.9.23.41.PM.mov

[melvin-bot]

Triggered auto assignment to @Luke9389 (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

[Luke9389]

I think we may actually want this for accessibility reasons. Perhaps @shawnborton knows offhand?

[Luke9389]

Like, when we tab around a page, we want a visual indication of what's focused.
Maybe the change here should be to un-focus on click, but again, not sure if that's actually a good practice.

[shawnborton]

Hmm I'm not sure, I think it should get the blue outline if it's tabbed into focus but maybe not if it's just clicked.

[melvin-bot]

Triggered auto assignment to @Christinadobrzyn (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

[melvin-bot]

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

[melvin-bot]

Current assignee @Luke9389 is eligible for the External assigner, not assigning anyone new.

[Luke9389]

Just made this external

[Christinadobrzyn]

Post job in Upwork

Internal posting - https://www.upwork.com/ab/applicants/1575712184410296320/job-details
External posting - https://www.upwork.com/jobs/~0123fb93d46cf2feb6

[getusha]

Proposal

We will get the exact behavior when we right click mouse on it too.

New.Expensify.1.mp4

Fixed by doing:
Check if isChecked and isFocused to apply the border.

App/src/components/Checkbox.js

Line 107 in 6bd5304

this.state.isFocused && styles.borderColorFocus,

TO
this.props.isChecked && this.state.isFocused && styles.borderColorFocus,

Result

2.New.Expensify.mp4

Also we will NOT have same behavior for clicking Checkbox and clicking on Member name.

[JmillsExpensify]

Nice! Let's get the party start.

[Christinadobrzyn]

Job posting - https://www.upwork.com/jobs/~0164f5f013388bbb18

Okay hiring in Upwork:

• @hungvu193 $2000 hired for the fix
• @parasharrajat $2000 hired as C+
• @Puneet-here $250 hired as reporter
• @getusha hire for $1000 - Can you please send me your upwork profile? I can't find it

Let me know if I'm missing something

[getusha]

Applied! @Christinadobrzyn

[Christinadobrzyn]

Perfect! Hired you @getusha - I'll send a payment to you once this job is closed. Thanks for all your help!

[parasharrajat]

Agreed with @Luke9389.

🎀 👀 🎀 C+ reviewed

[Christinadobrzyn]

Just checking in on this, it looks like the PR is here. Looks like we're waiting for something to be approved for merging, is that right? @Luke9389 @parasharrajat

[Luke9389]

Yep! I've just got one question on the PR, but we're close to merging.

[melvin-bot]

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

• Fix green outline at checkbox #13181

If no regressions arise, payment will be issued on 2022-12-20. 🎊

After the hold period, please check if any of the following need payment for this issue, and if so check them off after paying:

• External issue reporter
• Contributor that fixed the issue
• Contributor+ that helped on the issue and/or PR

As a reminder, here are the bonuses/penalties that should be applied for any External issue:

• Merged PR within 3 business days of assignment - 50% bonus
• Merged PR more than 9 business days after assignment - 50% penalty

[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:

• [@parasharrajat / @Luke9389] The PR that introduced the bug has been identified. Link to the PR:
• [@parasharrajat / @Luke9389] 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:
• [@parasharrajat / @Luke9389] 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:
• [@Christinadobrzyn] A regression test has been added or updated so that the same bug will not reach production again. Link to the GH issue for creating the test here: https://github.com/Expensify/Expensify/issues/250992

[Luke9389]

I'm not sure if this bug was actually caused by another PR. I think it's a bug in the sense that it's unwanted behavior, but not actually a regression. Do you agree @parasharrajat?

[Christinadobrzyn]

Paying this job,

I can't tell when the PR was merged for bonuses. It looks like it was created on Nov 30th and merged on Dec 3rd? Is that right?

Paid @Puneet-here as the reporter and @getusha $1000k but I'm trying to figure out the payment for @parasharrajat and @hungvu193 and if any bonuses apply, can you help? @parasharrajat and @Luke9389

[parasharrajat]

So here is the summary.

1. PR was c+ approved within 3 days Fix green outline at checkbox #13181 (review).
2. But CME took over in 6 days and then merged it.

So it was not merged in 3 days.

cc: @Luke9389

[Christinadobrzyn]

Job is paid for everyone - job closed in Upwork

@hungvu193 $2000 hired for the fix
@parasharrajat $2000 hired as C+
@Puneet-here $250 hired as reporter
@getusha hire for $1000 for contributions

@parasharrajat and @Luke9389 would you be able to take a look over this comment - #11283 (comment) and then close this issue?

[melvin-bot]

@shawnborton, @hungvu193, @parasharrajat, @Luke9389 Whoops! This issue is 2 days overdue. Let's get this updated quick!

[JmillsExpensify]

@Luke9389 @parasharrajat Mind helping @Christinadobrzyn with the checklist items above and then close out this issue?

[melvin-bot]

@shawnborton, @hungvu193, @parasharrajat, @Luke9389 6 days overdue. This is scarier than being forced to listen to Vogon poetry!

[Luke9389]

Just seeing this after being OOO in a while. I'll check it out.

[Luke9389]

I checked the boxes bc I'm pretty sure this was not a regression, so there was no link to give. I also don't think this warrants a discussion in expensify-bugs, we just need more thorough testing on front end UI PRs.