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

[$250] Workspace switcher - Selected filter is not highlighted #40454

Closed
6 tasks done
m-natarajan opened this issue Apr 18, 2024 · 20 comments
Closed
6 tasks done

[$250] Workspace switcher - Selected filter is not highlighted #40454

m-natarajan opened this issue Apr 18, 2024 · 20 comments
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Engineering External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review Weekly KSv2

Comments

@m-natarajan
Copy link

m-natarajan commented Apr 18, 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: 1.4.63-0
Reproducible in staging?: y
Reproducible in production?: no
If this was caught during regression testing, add the test name, ID and link from TestRail: n/a
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Expensify/Expensify Issue URL:
Issue reported by: applause internal team
Slack conversation:

Action Performed:

  1. Go to staging.new.expensify.com
  2. Open workspace switcher.

Expected Result:

The selected filter will be highlighted (production behavior).

Actual Result:

The selected filter is not highlighted.

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

Add any screenshot/video evidence

Bug6453410_1713432881205.20240418_173309.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01cffadd881b76c8f7
  • Upwork Job ID: 1781065373142794240
  • Last Price Increase: 2024-04-18
  • Automatic offers:
    • s77rt | Reviewer | 0
    • gijoe0295 | Contributor | 0
    • DylanDylann | Contributor | 0
@m-natarajan m-natarajan added DeployBlockerCash This issue or pull request should block deployment Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Apr 18, 2024
Copy link

melvin-bot bot commented Apr 18, 2024

Triggered auto assignment to @marcochavezf (DeployBlockerCash), see https://stackoverflowteams.com/c/expensify/questions/9980/ for more details.

Copy link

melvin-bot bot commented Apr 18, 2024

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

Copy link
Contributor

👋 Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open `StagingDeployCash` deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:

  1. Identify the pull request that introduced this issue and revert it.
  2. Find someone who can quickly fix the issue.
  3. Fix the issue yourself.

@m-natarajan
Copy link
Author

m-natarajan commented Apr 18, 2024

@MitchExpensify FYI I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors.

@m-natarajan
Copy link
Author

We think this bug might be related to #vip-vsb

@marcochavezf
Copy link
Contributor

Looks like the bug was introduced in this PR

@marcochavezf
Copy link
Contributor

I can't create a straight revert from the offending PR, but yeah seems something's up with the new SelectionList component and the isSelected cc @lukemorawski @rushatgabhane @cristipaval

@marcochavezf
Copy link
Contributor

Making it external in case someone can spot the error

@marcochavezf marcochavezf added the External Added to denote the issue can be worked on by a contributor label Apr 18, 2024
Copy link

melvin-bot bot commented Apr 18, 2024

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

@melvin-bot melvin-bot bot changed the title Workspace switcher - Selected filter is not highlighted [$250] Workspace switcher - Selected filter is not highlighted Apr 18, 2024
@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Apr 18, 2024
Copy link

melvin-bot bot commented Apr 18, 2024

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

@ZhenjaHorbach

This comment has been minimized.

@gijoe0295
Copy link
Contributor

gijoe0295 commented Apr 18, 2024

Proposal

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

The selected filter is not highlighted.

We have the same problem with ShareLogPage.

What is the root cause of that problem?

Regression from #40179.

sections only gets full data when screen transition ends:

sections={didScreenTransitionEnd ? sections : CONST.EMPTY_ARRAY}

That made the check below reset focused index to -1 because the flattenedSections changed:

// Remove the focus if the search input is empty or selected options length is changed else focus on the first non disabled item
const newSelectedIndex = textInputValue === '' || flattenedSections.selectedOptions.length !== prevSelectedOptionsLength ? -1 : 0;

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

Pass sections directly instead of waiting till didScreenTransitionEnd:

sections={didScreenTransitionEnd ? sections : CONST.EMPTY_ARRAY}

I investigated the reason why this was added but found no clue. I also tested on different platforms and compared with production behavior, the changes only add the skeleton (loading) effect when we first open the switcher. IMO, app feels better without it.

With loading Without loading
Screen.Recording.2024-04-19.at.05.40.32-source.mov
Screen.Recording.2024-04-19.at.05.40.13-source.mov

What alternative solutions did you explore? (Optional)

We can take into account the case when sections/flattenedSections first gets data (changes from empty array/undefined) and return early instead of reset focused index:

// Avoid changing focus if the textInputValue remains unchanged.
if ((prevTextInputValue === textInputValue && flattenedSections.selectedOptions.length === prevSelectedOptionsLength) || flattenedSections.allOptions.length === 0) {
return;
}
// Remove the focus if the search input is empty or selected options length is changed else focus on the first non disabled item
const newSelectedIndex = textInputValue === '' || flattenedSections.selectedOptions.length !== prevSelectedOptionsLength ? -1 : 0;

@marcochavezf
Copy link
Contributor

Thanks for the proposals. @gijoe0295's solution looks good, I also agree that looks better without the loading effect. I think didScreenTransitionEnd was copied from another page component but is not required for the workspace switcher.

I'm going to create a quick PR to fix this deploy blocker since I already made the changes while I was testing the proposals, but I'm going to assign you for the compensation

@melvin-bot melvin-bot bot removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Apr 19, 2024
Copy link

melvin-bot bot commented Apr 19, 2024

📣 @s77rt 🎉 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 Apr 19, 2024

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

@melvin-bot melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 and removed Hourly KSv2 labels Apr 19, 2024
@mountiny mountiny assigned DylanDylann and unassigned s77rt Apr 19, 2024
Copy link

melvin-bot bot commented Apr 19, 2024

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

@thienlnam
Copy link
Contributor

This has been CPed to staging

@thienlnam thienlnam removed the DeployBlockerCash This issue or pull request should block deployment label Apr 20, 2024
@gijoe0295
Copy link
Contributor

@MitchExpensify I think it's time for payment for me and @DylanDylann, isn't it?

@MitchExpensify
Copy link
Contributor

MitchExpensify commented May 6, 2024

Payment summary:

$250 C+ @DylanDylann Upwork
$250 C @gijoe0295 Upwork

@MitchExpensify
Copy link
Contributor

Paid and contract ended

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something is broken. Auto assigns a BugZero manager. Engineering External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review Weekly KSv2
Projects
None yet
Development

No branches or pull requests

8 participants