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] Search filters - Scrolling behavior after selecting currency and category is different #47058

Closed
6 tasks done
izarutskaya opened this issue Aug 8, 2024 · 19 comments
Closed
6 tasks done
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

@izarutskaya
Copy link

izarutskaya commented Aug 8, 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: v9.0.18-1
Reproducible in staging?: Y
Reproducible in production?: N
Found when executing PR : #46566
Email or phone of affected tester (no customers): applausetester+kh050806@applause.expensifail.com
Logs: https://stackoverflow.com/c/expensify/questions/4856
Issue reported by: Applause-Internal team

Action Performed:

  1. Launch New Expensify app.
  2. Navigate to https://staging.new.expensify.com/search/filters
    via deep link.
  3. Tap Currency.
  4. Scroll down and select a currency.
  5. Note that the list does not scroll up after selecting a currency.
  6. Tap Category.
  7. Scroll down and select a category.
  8. Note that the list scrolls up after selecting a category.
  9. Repeat the steps above on web.

Expected Result:

The scrolling behavior in filters should be the same after selecting currency and category.

Actual Result:

On Android and iOS,
In Step 5, the list does not scroll up after selecting a currency.
In Step 8, the list scrolls up after selecting a category.

On web, mweb and desktop app,
In Step 5, the list scrolls up without animation after selecting a currency.
In Step 8, the list scrolls up with animation after selecting a category.

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

Bug6565464_1723112060019.web.mp4
Bug6565464_1723112060032.iOS.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01f10b9412daa7bb88
  • Upwork Job ID: 1821575106946228601
  • Last Price Increase: 2024-08-08
  • Automatic offers:
    • dukenv0307 | Reviewer | 103492530
    • dominictb | Contributor | 103492533
Issue OwnerCurrent Issue Owner: @dukenv0307
@izarutskaya izarutskaya added DeployBlockerCash This issue or pull request should block deployment Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. DeployBlocker Indicates it should block deploying the API labels Aug 8, 2024
Copy link

melvin-bot bot commented Aug 8, 2024

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

melvin-bot bot commented Aug 8, 2024

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

@github-actions github-actions bot added Engineering Hourly KSv2 and removed Daily KSv2 labels Aug 8, 2024
Copy link
Contributor

github-actions bot commented Aug 8, 2024

👋 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.

@izarutskaya
Copy link
Author

We think this issue might be related to the #collect project.

@carlosmiceli carlosmiceli added External Added to denote the issue can be worked on by a contributor and removed DeployBlockerCash This issue or pull request should block deployment DeployBlocker Indicates it should block deploying the API labels Aug 8, 2024
@melvin-bot melvin-bot bot changed the title Search filters - Scrolling behavior after selecting currency and category is different [$250] Search filters - Scrolling behavior after selecting currency and category is different Aug 8, 2024
Copy link

melvin-bot bot commented Aug 8, 2024

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

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Aug 8, 2024
Copy link

melvin-bot bot commented Aug 8, 2024

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

@carlosmiceli carlosmiceli added Daily KSv2 and removed Hourly KSv2 labels Aug 8, 2024
@dominictb
Copy link
Contributor

dominictb commented Aug 8, 2024

Edited by proposal-police: This proposal was edited at 2024-08-08 16:58:59 UTC.

Proposal

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

On Android and iOS,
In Step 5, the list does not scroll up after selecting a currency.
In Step 8, the list scrolls up after selecting a category.

On web, mweb and desktop app,
In Step 5, the list scrolls up without animation after selecting a currency.
In Step 8, the list scrolls up with animation after selecting a category.

What is the root cause of that problem?

We only do the scroll-with-animation after an option is selected, if the selection list has more than 1 section

scrollToIndex(Math.max(selectedOptionsCount - 1, 0), true);
, the reason being that for single section selection list, usually we don't sort the selected items on top because there are not that many items. So as category is a multi-section selection list, it will has scroll-with-animation, while currency list is a single-section list so it won't have.

However, the currency list in the search filter is different from other single-section list, we actually do sort the selected items on top

if (currencyA.isSelected === currencyB.isSelected) {
, so it's necessary to have scroll-with-animation like other multi-section list which also sorts selected items on top.

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

We need to make sure the currency list which has canSelectMultiple as true, have 2 sections (selected section and remaining section), like any other multi selection list here https://github.com/Expensify/App/blob/main/src/pages/Search/SearchFiltersCategoryPage.tsx#L74-L83

So we can update this block to

let sections = [];

if (canSelectMultiple) {
    sections.push({
        title: undefined,
        data: filteredCurrencies.filter(currency => currency.isSelected),
    });
    sections.push({
        title: undefined,
        data: filteredCurrencies.filter(currency => !currency.isSelected),
    });
} else {
    sections = [{
        data: filteredCurrencies,
    }];
}

return {
    sections: isEmpty
        ? []
        : sections,

We can apply other sorting criteria to filteredCurrencies, before generating the sections, if necessary

What alternative solutions did you explore? (Optional)

@dominictb
Copy link
Contributor

Proposal updated to add an enhancement

@dukenv0307
Copy link
Contributor

dukenv0307 commented Aug 9, 2024

@dominictb What about On web, mweb and desktop app issue? I see you just fix the problem on Android and iOS

@dukenv0307
Copy link
Contributor

On web, why the list scrolls up without animation after selecting a currency

@dominictb
Copy link
Contributor

dominictb commented Aug 10, 2024

@dukenv0307 Thanks for the feedback. I've updated the proposal, let me know if anything is still unclear.

It works now for all platforms.

@dukenv0307
Copy link
Contributor

Thank you. Let's go with @dominictb's proposal, it looks good and tests well.

🎀👀🎀 C+ reviewed

Copy link

melvin-bot bot commented Aug 12, 2024

Current assignee @carlosmiceli is eligible for the choreEngineerContributorManagement assigner, not assigning anyone new.

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

melvin-bot bot commented Aug 12, 2024

📣 @dukenv0307 🎉 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 Aug 12, 2024

📣 @dominictb 🎉 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 Daily KSv2 labels Aug 14, 2024
@RachCHopkins
Copy link
Contributor

Has the automation failed again @carlosmiceli? Is this done and ready to be paid?

@carlosmiceli
Copy link
Contributor

Yeah, looks like it... PR is already in prod, so yeah, this should be done.

@RachCHopkins
Copy link
Contributor

Payment Summary:

Upwork job here

@RachCHopkins
Copy link
Contributor

Contributors have been paid, the contracts have been completed, and the Upwork post has been closed.

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

5 participants