Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
3.56k stars 2.9k forks source link

[HOLD for production 2024-10-07][$250] [Search v2.3] Add the bookmark icon for Saved searches on mobile #49442

Closed lakchote closed 3 weeks ago

lakchote commented 2 months ago

See https://github.com/Expensify/App/pull/48566#issuecomment-2350223656

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~021836682788967357264
  • Upwork Job ID: 1836682788967357264
  • Last Price Increase: 2024-09-19
Issue OwnerCurrent Issue Owner: @VictoriaExpensify
melvin-bot[bot] commented 2 months ago

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

melvin-bot[bot] commented 2 months ago

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

mkzie2 commented 2 months ago

Proposal

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

Add the bookmark icon for Saved searches on mobile

What is the root cause of that problem?

On mobile, we display the saved search query that we're viewing here

https://github.com/Expensify/App/blob/37bf6b2fef128786dfcd83257a56f56c0cd1eb3d/src/pages/Search/SearchTypeMenuNarrow.tsx#L87

And we don't add icon field for the saved searches item https://github.com/Expensify/App/blob/37bf6b2fef128786dfcd83257a56f56c0cd1eb3d/src/pages/Search/SearchTypeMenuNarrow.tsx#L108

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

We should remove this item here because it will be included in the savedSearchItems below or we can exclude the viewing saved search in savedSearchItems

https://github.com/Expensify/App/blob/37bf6b2fef128786dfcd83257a56f56c0cd1eb3d/src/pages/Search/SearchTypeMenuNarrow.tsx#L87

Add icon field as Expensicons.BookMark here, we can also add other icon config field if it needs

icon: Expensicons.Bookmark,

https://github.com/Expensify/App/blob/37bf6b2fef128786dfcd83257a56f56c0cd1eb3d/src/pages/Search/SearchTypeMenuNarrow.tsx#L108

What alternative solutions did you explore? (Optional)

thesahindia commented 2 months ago

We should remove this item here because it will be included in the savedSearchItems below or we can exclude the viewing saved search in savedSearchItems

The solution for adding icon is fine but I am not sure I understand what you said above. Could you explain it?

mkzie2 commented 2 months ago

If we're viewing a saved search, we shouldn't add the custom query option to the bottom modal. Example: In the imagine below, the type:expense status:all currency:ALL AMD shouldn't be shown

As the ref comment here mentioned, we shouldn't display the viewing saved search query in popoverMenuItems or savedSearchItems

thesahindia commented 2 months ago

Got it!

thesahindia commented 2 months ago

@mkzie2's proposal looks fine to me!

🎀 👀 🎀 C+ reviewed

melvin-bot[bot] commented 2 months ago

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

lakchote commented 1 month ago

@mkzie2's proposal LGTM.

melvin-bot[bot] commented 1 month ago

📣 @mkzie2 You have been assigned to this job! Please apply to the Upwork job and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻 Once you apply to this job, your Upwork ID will be stored and you will be automatically hired for future jobs! Keep in mind: Code of Conduct | Contributing 📖

mkzie2 commented 1 month ago

Will raise the PR soon.

mkzie2 commented 1 month ago

@thesahindia The PR is ready https://github.com/Expensify/App/pull/49583.

melvin-bot[bot] commented 1 month ago

This issue has not been updated in over 15 days. @lakchote, @luacmartins, @thesahindia, @mkzie2 eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!

mkzie2 commented 1 month ago

@lakchote Can we process payment here?

melvin-bot[bot] commented 1 month ago

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

lakchote commented 1 month ago

@VictoriaExpensify please process payment for @mkzie2, thanks!

melvin-bot[bot] commented 3 weeks ago

@lakchote, @luacmartins, @VictoriaExpensify, @thesahindia, @mkzie2 Whoops! This issue is 2 days overdue. Let's get this updated quick!

luacmartins commented 3 weeks ago

Waiting on payment

VictoriaExpensify commented 3 weeks ago

Hey @mkzie2 - can you please accept the job offer and I'll organise payment https://www.upwork.com/nx/wm/offer/104530588

mkzie2 commented 3 weeks ago

@VictoriaExpensify Did it thank you

thesahindia commented 3 weeks ago

Hey @VictoriaExpensify, I am eligible for the C+ compensation. I will request it on new dot. Could you please add the payment summary here? I will need it to get the money request approved.

VictoriaExpensify commented 3 weeks ago

Payment summary: Contributor: @mkzie2 - Paid $250 via Upwork Contributor+: @thesahindia - owed $250 via NewDot