-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
[Hold for Payment 2024-09-19][$250][Search v2.1] Filters button should not be shown on Search page during "select mode". #48511
Comments
Triggered auto assignment to @mallenexpensify ( |
Edited by proposal-police: This proposal was edited at 2024-09-07 20:52:11 UTC. ProposalPlease re-state the problem that we are trying to solve in this issue.What is the root cause of that problem?Filters button should not be shown on Search page during "select mode". What changes do you think we should make in order to solve the problem?currently the filter button is always shown App/src/components/Search/SearchPageHeader.tsx Lines 314 to 319 in adc29ab
What alternative solutions did you explore? (Optional)We should modify this to ensure the button is only shown if or Alternatively, we can use {selectedTransactionsKeys.length === 0 && (
<Button
text={translate('search.filtersHeader')}
icon={Expensicons.Filters}
onPress={() => Navigation.navigate(ROUTES.SEARCH_ADVANCED_FILTERS)}
medium
/>
)} POCScreen.Recording.2024-09-03.at.23.22.35.mov |
ProposalPlease re-state the problem that we are trying to solve in this issue.Filters button should not be shown on Search page during "select mode". What is the root cause of that problem?We show the selected button if App/src/components/Search/SearchPageHeader.tsx Lines 303 to 319 in 6e500c2
What changes do you think we should make in order to solve the problem?We should update it to if-else condition, to show the selected button if {headerButtonsOptions.length > 0 ? (
<ButtonWithDropdownMenu
onPress={() => null}
shouldAlwaysShowDropdownMenu
pressOnEnter
buttonSize={CONST.DROPDOWN_BUTTON_SIZE.MEDIUM}
customText={translate('workspace.common.selected', {selectedNumber: selectedTransactionsKeys.length})}
options={headerButtonsOptions}
isSplitButton={false}
/>
) : (
<Button
text={translate('search.filtersHeader')}
icon={Expensicons.Filters}
onPress={() => Navigation.navigate(ROUTES.SEARCH_ADVANCED_FILTERS)}
medium
/>
)} What alternative solutions did you explore? (Optional) |
Hi, I'm Wiktor Gut from SWM agency, please assign me this issue! |
Job added to Upwork: https://www.upwork.com/jobs/~021832173197655902215 |
Triggered auto assignment to Contributor-plus team member for initial proposal review - @jjcoffee ( |
@jjcoffee plz review the proposals above, thx |
Updated proposalAdded a minor clarification explaining why I used the |
Hi @luacmartins, can you please take a peek at this bug and assign it? It's a pretty small thing connected directly to Search, so I'd love to handle it quickly as part of SWM 🙏 |
Sounds like @Guccio163 is going to work on this! 🎀👀🎀 C+ reviewed |
Triggered auto assignment to @thienlnam, see https://stackoverflow.com/c/expensify/questions/7972 for more details. |
I think the existing proposals need to be reviewed first base on this contributing guideline |
@nyomanjyotisa Oh sorry, I hadn't encountered that before! In that case both proposals are basically the same, so I think we're good to go with @abzokhattab's proposal since his was first. 🎀👀🎀 C+ reviewed |
Current assignee @thienlnam is eligible for the choreEngineerContributorManagement assigner, not assigning anyone new. |
I believe the if-else condition proposed in my solution is necessary, since we want to prevent redundant/unnecessary checks and optimize performance |
Sorry everyone, this is part of the Search project and @Guccio163 is working on this already. |
@abzokhattab and @nyomanjyotisa , once the linked PR has been on production for 7 days, without a regression, I'll review this issue for potential payment. One key point to consider here is this from CONTRIBUTING.md .
That doesn't guarantee that no one else will be eligible for compensation, I'll update everyone at the end. |
@luacmartins I think you closed this by mistake? |
Oh for some reason my merge commit closed the issue. Reopening. |
Deployed to production Sept 12 so payment should be due 2024-09-19. cc @mallenexpensify |
Regression Test Proposal
Do we agree 👍 or 👎 |
Thanks! No need for regression tests at this time, we'll add them as part of the project wrap up |
Contributor: @jjcoffee paid $250 via Upwork Thx! |
@mallenexpensify Any update about potential payment based on this? |
same, since my proposal was the selected solution. please let us know about any potential payments. Thx!! |
@nyomanjyotisa @abzokhattab sorry, but this was within the regression period and the Search project is being handled by SWM and myself, so we won't be able to issue additional payments at this time. |
To provide more details...
Based on the above, compensation isn't due. |
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.28-2
Reproducible in staging?: Y
Reproducible in production?: Y
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: @dannymcclain
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1725380044951249
Action Performed:
Expected Result:
Filters button should not be shown on search during select mode
Actual Result:
Filters button are shown on search during select mode
Workaround:
Unknown
Platforms:
Which of our officially supported platforms is this issue occurring on?
Screenshots/Videos
Add any screenshot/video evidence
View all open jobs on GitHub
Upwork Automation - Do Not Edit
Issue Owner
Current Issue Owner: @mallenexpensifyThe text was updated successfully, but these errors were encountered: