[HOLD for payment 2023-09-04] Selection List Refactor: Checkbox List

[thiagobrez]

This issue keeps track of Phase 2 of the Selection List Refactor, in which we are refactoring all different list component variations into 3 new, clean components:

• Phase 1: Radio Button List
• Phase 2: Checkbox List (this phase)
• Phase 3: Simple Selection List

Thoroughly discussed in the parent issue: #11795 (comment)

[cristipaval]

I guess @thiagobrez has not started on this one yet.

[thiagobrez]

Correct @cristipaval. I'll send the PR for the first refactor tomorrow. Then I'll start on this one.

[thiagobrez]

Starting to investigate this.

[thiagobrez]

Hey @shawnborton! I need your input here. Regarding multiple selection lists: most of them use radio buttons, but there are also cases of checkboxes. I was planning to standardise and leave only the checkbox version, as I feel like it's more intuitive for multiple selection. Do you agree, or do you have new designs/thoughts here?

[shawnborton]

Can you show me what you are referring to? I agree that we need to do a better job of making multiple selections look like checkboxes and single selections (though they don't really exist in the app yet) look like radios.

[thiagobrez]

@shawnborton My bad, forgot to attach screenshots 🤦🏻‍♂️

This is what I was referring to:

Workspace manage members	Workspace invite members

[shawnborton]

Got it. I think we should just leave those untouched for now and focus only on existing checkboxes. I think we will examine the overall pattern and relationship between checkboxes and list views at some point in the future.

[thiagobrez]

Weekly update: got the base structure working alongside Manage Workspace Members List. Iterated on top of the new component introduced in Phase 1 changes (PR still being reviewed here), so that needs to be merged before raising a PR for this one.

Next week will work on the remaining multiple selection lists, which should be quicker now that the base is ready.

[thiagobrez]

Hey @cristipaval! I noticed that I'm not assigned to this issue. Just pointing out because I know you guys use it for internal processes :)

[cristipaval]

Good point. You're now assigned.

[thiagobrez]

Weekly update: all multiple selection lists are working! 🎉

That is:

• Workspace members page
• Workspace invite members page
• Split bill page
• New group page

Next steps are to improve the Storybook stories and performance tests built in Phase 1 PR, which is still being reviewed. Once that is merged, I can rebase into this one and raise the PR for Phase 2.

[thiagobrez]

@shawnborton Current state of multiple selection lists:

Workspace members and invite

Before, workspace members page and workspace invite page were using 2 different lists. Members page was using checkboxes while invite page was using radio buttons. Members page had no keyboard controls, while invite page had.

Now, both are standardised using the same component, all checkboxes with keyboard controls.

Would like your input specially regarding disabled items (admin, for example):

• Previous behavior was that it was still selectable, and showed an error when selected. In my opinion this is unnecessary because it's one extra step for the user to discover that his action will not be possible. Also, it causes a weird behavior with the "Select All" checkbox, since you can manually select that item, but when checking "Select All", that item wasn't selected

• Current behavior is that items that can't be selected are disabled upfront (and indicated with opacity style). This makes it obvious for the user that it can't be selected, and prevents any weirdness with the "Select All" checkbox, since it can never be selected

What do you think, do you agree? See videos below

Before	Current
before.mov	after.mov

Split bill

Standardised using new checkbox list component

split.bill.mov

New group

Standardised using new checkbox list component

new.group.mov

[shawnborton]

Hmm I don't think we should make the whole admin row appear "disabled" though - but rather, let's just make the checkbox part look disabled. Otherwise we are competing with some offline patterns we use here, cc @trjExpensify

As for the others, I don't think we should use these kinds of checkboxes on the split or group pages yet. Let's leave them as-is. I left that same comment for you above.

[thiagobrez]

Hmm I don't think we should make the whole admin row appear "disabled" though - but rather, let's just make the checkbox part look disabled. Otherwise we are competing with some offline patterns we use here

Ok, cool 👍🏻

As for the others, I don't think we should use these kinds of checkboxes on the split or group pages yet. Let's leave them as-is. I left that #20353 (comment) for you above.

My mistake, I understood it differently when I read it the first time. Will leave as-is

[trjExpensify]

Otherwise we are competing with some offline patterns we use here, cc @trjExpensify

Totally agree, that's the "pending create" pattern used when adding a member with "offline pattern B". 👍

[thiagobrez]

Had to focus on a higher priority issue for the last days. Coming back to this on Monday, very close to raising a PR

[thiagobrez]

Update: Draft PR is being reviewed internally

[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 1.3.57-6 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• refactor: SelectionList multiple selection #22622

If no regressions arise, payment will be issued on 2023-09-04. 🎊

After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.

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

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

• @thiagobrez does not require payment (Contractor)
• @Santhosh-Sellavel does not require payment (Eligible for Manual Requests)

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]

Triggered auto assignment to @slafortune (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

[melvin-bot]

Bug0 Triage Checklist (Main S/O)

• This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
• This bug is not a duplicate report (check E/App issues and #expensify-bugs)
  • If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
• This bug is reproducible using the reproduction steps in the OP. S/O
  • If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
  • If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
• This issue is filled out as thoroughly and clearly as possible
  • Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
• I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

[cristipaval]

Bug0 Triage Checklist (Main S/O)

• This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)

• This bug is not a duplicate report (check E/App issues and #expensify-bugs)

  • If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead

• This bug is reproducible using the reproduction steps in the OP. S/O

  • If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
  • If the reproduction steps aren't clear and you determine the correct steps, please update the OP.

• This issue is filled out as thoroughly and clearly as possible

  • Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.

• I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

Ignore this @slafortune , I added Bug label for the payments.

[cristipaval]

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

• refactor: SelectionList multiple selection #22622

If no regressions arise, payment will be issued on 2023-09-04. 🎊

After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.

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

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

• @thiagobrez does not require payment (Contractor)
• @Santhosh-Sellavel does not require payment (Eligible for Manual Requests)

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

FYI @slafortune: There were regressions after this PR because I wanted to merge it even though it was not perfect. The PR became too big and conflicts were regularly occurring with the main branch. I decided to merge it as long as there were no blockers for the users. All regressions (mostly styling issues) were quickly fixed in follow-up PRs. So please don't apply penalties to @Santhosh-Sellavel. He did a great job testing that huge PR.

[Santhosh-Sellavel]

@cristipaval This is a huge PR and there were 4 follow-ups till two got merged & two were in review two more PRs to come can we increase the bounty to $3k or at least $2K thanks!

[slafortune]

Sorry for the delay on this @Santhosh-Sellavel. While the PR was big, it appears to have been more fragmented rather than causing a huge time suck and blocking you on other things.

[slafortune]

Payment summary -

Bug reporter - NA
@thiagobrez does not require payment (Contractor)
@Santhosh-Sellavel is Eligible for Manual Requests and owed $1000

[Santhosh-Sellavel]

Sorry for the delay on this @Santhosh-Sellavel. While the PR was big, it appears to have been more fragmented rather than causing a huge time suck and blocking you on other things.

I disagree it might look like but that's not the case here. It directly affects certain areas covered in this phase, as well affects the changes done in this first phase. So a lot of time of invested in the Review & testing.

cc: @cristipaval Let me know your thoughts, thanks!

[slafortune]

@Santhosh-Sellavel We want to clarify that the advertised price for the job is non-negotiable. We do not review or adjust the payment for smaller PRs either. This decision is final, and we will proceed accordingly. If you have any further questions or concerns unrelated to the price, please feel free to discuss them.

[Santhosh-Sellavel]

@slafortune

Fine I'll settle for the $1K here if it's final decision.

But we do change the price based on complexity or it's time consuming. There is no advertised price for the job. We had a fixed price a $1000 C+ per internal/agency PR review at that point.

Only certain PR qualifies for price revisions if it's approved by CME/BZ.

Thanks!

[slafortune]

Payment Summary -

Bug reporter - NA
@thiagobrez does not require payment (Contractor)
@Santhosh-Sellavel is Eligible for Manual Requests and owed $1000

cc @anmurali @JmillsExpensify

[JmillsExpensify]

$1,000 payment approved for @Santhosh-Sellavel based on BZ summary.