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

[Search v2.4] Search Router Polish Issues #50250

Open
9 of 11 tasks
luacmartins opened this issue Oct 4, 2024 · 18 comments
Open
9 of 11 tasks

[Search v2.4] Search Router Polish Issues #50250

luacmartins opened this issue Oct 4, 2024 · 18 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Daily KSv2 Overdue

Comments

@luacmartins
Copy link
Contributor

luacmartins commented Oct 4, 2024

@luacmartins luacmartins added the Daily KSv2 label Oct 4, 2024
@luacmartins luacmartins self-assigned this Oct 4, 2024
@luacmartins luacmartins changed the title Search Router Polish Issues [Search v2.4] Search Router Polish Issues Oct 4, 2024
@SzymczakJ
Copy link
Contributor

Hey! I’m Jakub Szymczak from Software Mansion, an expert agency, and I’d like to work on this issue!

@luacmartins
Copy link
Contributor Author

@SzymczakJ just an FYI we're still adding more items to this issue as we test the feature

@SzymczakJ
Copy link
Contributor

we're still adding more items

Then I'll focus on other issues and come back to this PR later.

@melvin-bot melvin-bot bot added the Overdue label Oct 10, 2024
@luacmartins
Copy link
Contributor Author

Added a few more items to the list

@melvin-bot melvin-bot bot removed the Overdue label Oct 10, 2024
@289Adam289
Copy link
Contributor

I see the list is getting longer. I can also help with polish issues and follow ups from #49838

@289Adam289
Copy link
Contributor

Regarding

Clicking on a contextual filter in the search results takes you back to the default filters, rather than adding an extra search query to the existing search results. We need to preserve the current context and just add/update the type/status

#49838 is solving this issue for status contextual filters. The reason type contextual filters reset query to default values is that lists of filters for type:expense and type:chat are very different. Similar behavior can be observed on github when changing between issues and prs.
If we want to preserve query when changing a type should we remove irrelevant filters (e.g. merchant in type:chat) or leave it as is.

@luacmartins
Copy link
Contributor Author

I think that's fine for status for now. For type, we might wanna update the API/App to just ignore the irrelevant filters.

Copy link

melvin-bot bot commented Oct 14, 2024

@luacmartins, @SzymczakJ Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

@Kicu
Copy link
Contributor

Kicu commented Oct 15, 2024

Hey I'd be happy to start working on some of these, as for now v2.5 is still being in the planning mode, so I think it's great time to do some polishing.

In addition Search seems like an important part of the App so I would like to also spend a bit of time on pure code refactors (if thats fine with you). Cleaner code should allow for faster iteration of features, and it would be nice to avoid cases similar to ReportUtils having 8000+ loc that everyone is afraid of touching 😅


Also this one:

CMD + K should close the modal when it's already open

will be fixed as soon as #49984 is merged, as Jakub fixed it.

@Kicu
Copy link
Contributor

Kicu commented Oct 15, 2024

Another thing I wanted to confirm before actual implementation starts.
About this:

Clicking on a contextual filter in the search results takes you back to the default filters, rather than adding an extra search query to the existing search results.

see also this comment by Adam: #50250 (comment)

So originally (a long time ago in a galaxy far far away 😅) we had this design:

So we used to have logic where everything on the left side is just a "link" of sorts, so the Chats, Expenses etc and also Saved Searches kinda behave the same way.
And right now we're landing somewhere in the middle:

  • status tabs are next to results and should change the current query (makes sense ✅)
  • Expenses, Chats, etc.. are not next to results but they should also change the current query? 🤔
  • Saved searches will move the user to a fresh query (also makes sense ✅)

Is this what we want to move forward with? Or should we bring back the dedicated type tabs?

CC @luacmartins @JmillsExpensify @shawnborton

@luacmartins
Copy link
Contributor Author

Is this what we want to move forward with? Or should we bring back the dedicated type tabs?

We're descoping the decidated type tabs from v2.5 and I think for now the plan is to go with the current behavior, i.e. the LHN items navigate you to a brand new search and the status buttons update the status on the current custom search you're viewing. I think we can do nothing on this one for now. @JmillsExpensify for confirmation.

@melvin-bot melvin-bot bot removed the Overdue label Oct 15, 2024
@289Adam289 289Adam289 mentioned this issue Oct 16, 2024
50 tasks
@SzymczakJ SzymczakJ mentioned this issue Oct 16, 2024
48 tasks
@289Adam289
Copy link
Contributor

289Adam289 commented Oct 16, 2024

Remove unnecessary accountID lookup

Regarding this issue I believe accountID lookup is necessary for advanced filters from and to to work, because they are based on IDs. Current implementation replaces all emails that are present in onyx with IDs so that they appear as selected values in advanced filters and remaining emails are sent to backend unchanged.

We can try to refactor filters from and to to use emails not IDs that could make removing accountID lookup possible.

@luacmartins
Copy link
Contributor Author

That one is more of a cleanup sine the API can now take emails directly too. I'd prioritize the other issues first (and autocomplete) and maybe we can even descope it

@melvin-bot melvin-bot bot added Weekly KSv2 and removed Weekly KSv2 labels Oct 18, 2024
@luacmartins
Copy link
Contributor Author

luacmartins commented Oct 21, 2024

I couldn't reproduce BUG: I’m searching in a chat that has messages and getting no results on the search results page when I land there. in my tests today. I created a separate issue for it, so I'll remove it from the OP.

@melvin-bot melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Weekly KSv2 labels Oct 22, 2024
@melvin-bot melvin-bot bot changed the title [Search v2.4] Search Router Polish Issues [HOLD for payment 2024-10-29] [Search v2.4] Search Router Polish Issues Oct 22, 2024
@melvin-bot melvin-bot bot removed the Reviewing Has a PR in review label Oct 22, 2024
Copy link

melvin-bot bot commented Oct 22, 2024

Reviewing label has been removed, please complete the "BugZero Checklist".

Copy link

melvin-bot bot commented Oct 22, 2024

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

If no regressions arise, payment will be issued on 2024-10-29. 🎊

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

  • @SzymczakJ does not require payment (Contractor)

@luacmartins luacmartins changed the title [HOLD for payment 2024-10-29] [Search v2.4] Search Router Polish Issues [Search v2.4] Search Router Polish Issues Oct 22, 2024
@melvin-bot melvin-bot bot added Daily KSv2 Overdue and removed Weekly KSv2 labels Oct 28, 2024
@luacmartins
Copy link
Contributor Author

PR addressing most of these issues was merged.

@melvin-bot melvin-bot bot added Overdue and removed Overdue labels Oct 29, 2024
Copy link

melvin-bot bot commented Nov 1, 2024

@luacmartins, @SzymczakJ Whoops! This issue is 2 days overdue. Let's get this updated quick!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Daily KSv2 Overdue
Projects
Status: Done
Development

No branches or pull requests

4 participants