feat: [M3-8229] - Volume & Images search and filtering #10570
Conversation
| } | ||
|
|
||
| setQuery(selectedImageFromParams.label); | ||
| }, [imageIdFromParam, manualImages, automaticImages]); |
There was a problem hiding this comment.
This may feel verbose but i like our trend of being super explicit in useEffects (and in logic blocks in general) to make things clear and readable, including early returns when possible.
| placeholder="Search Images" | ||
| sx={{ mb: 2 }} | ||
| value={query} | ||
| /> |
There was a problem hiding this comment.
Why did I not use the DebouncedSearchTextField? I am glad you asked. Cause that component is essentially crap and needs a complete refactor. There's even a customValue on that component to bypass the debouncing which does not make sense at all. All we need is to debounce on the onChange which is done here directly.
We already have a ticket to refactor the DebouncedSearchTextField component but I am wondering if we need a component at all for this. TBD
| @@ -83,10 +85,9 @@ export const imageToSearchableItem = (image: Image): SearchableItem => ({ | |||
| data: { | |||
| created: image.created, | |||
| description: image.description || '', | |||
| /* TODO: Update this with the Images icon! */ | |||
There was a problem hiding this comment.
5 years later, yay! 🎉 (we were still showing the volume icon for images)
|
Coverage Report: ✅ |
| {isFetching ? ( | ||
| <TableRowLoading columns={6} /> | ||
| ) : ( |
There was a problem hiding this comment.
I'd be in favor of removing this loading state. I think the spinner on the Search field is good enough, this one just causes the page to shift more
There was a problem hiding this comment.
Am going to keep it for now and see about more feedback. On the fence, it's nice for large accounts where the pagination shows
There was a problem hiding this comment.
Still feel like we could do without this. It also causes shifting when the order is changed, which we don't do elsewhere in the app:
Screen.Recording.2024-06-14.at.10.26.41.AM.mov
There was a problem hiding this comment.
yeah i took it out but the search spinner isn't enough IMO - that's a bigger problem with a missing pattern in the app for this case - some tables have a circular loader for this case but that's annoying as well. i'll live with this
|
The failing See images below.
|
|
Thanks for the continued reviews and pushing for cleanest solution @bnussman-akamai 🥇 |
|
No problem! Thanks for jumping on this so quickly! 🛻💨 |
* Initial commit: search implementation * Cleaner logic * improve code and expand to images * e2e coverage * cleanup * type fixes post rebase * feedback @bnussman-akamai * Added changeset: Volume & Images landing pages search and filtering * feedback @bnussman-akamai * more changes from feedback * cleanup * fix empty state * moar cleanup * moar cleanup * code readability
Description 📝
This PR implements search/filtering for both the /volumes & /images landing pages.
The context surrounding this feature is due to the fact that we don't have landing pages for both those entities. When searching through the main search bar, while the entities return in the search suggestions, clicking on them will bring the user to the landing page without any filtering (user would have to do a text search on the page to locate the record). For large accounts, the problem is even worse since the record can be paginated and not visible on the page.
In order to improve this experience, the PR adds search capability on the landing pages, with a query param lookup to auto-populate the field when a main search bar search points to a record.
For coverage, the PR implements two new e2e tests as they are more reliable than units for this particular behavior (API filtering) and mocking wouldn't add too much value.
Changes 🔄
For both images and volumes:
Target release date 🗓️
6/24/2024
Preview 📷
Screen.Recording.2024-06-13.at.12.12.12.mov
Screen.Recording.2024-06-13.at.12.15.04.mov
How to test 🧪
Reproduction steps
Verification steps
ℹ️: images feature quite a constricting rate limiting. While debounced, over searching may get you a 400. This is a known issue which hopefully can be addressed soon (will have an ARB ticket for this).
As an Author I have considered 🤔
Check all that apply