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

refactor(web): search box #7397

Merged
merged 10 commits into from Feb 26, 2024
Merged

refactor(web): search box #7397

merged 10 commits into from Feb 26, 2024

Conversation

danieldietzler
Copy link
Member

@danieldietzler danieldietzler commented Feb 24, 2024

The GET /search/suggestions endpoint has been updated and returns now an object of all suggestions. As a result, the SearchSuggestionsType has been removed.

Copy link

cloudflare-pages bot commented Feb 24, 2024

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: 758216c
Status: ✅  Deploy successful!
Preview URL: https://59b3c3ff.immich.pages.dev
Branch Preview URL: https://refactor-search-box.immich.pages.dev

View logs

Copy link
Contributor

@jrasm91 jrasm91 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we should return all of these results in a single request.

@danieldietzler danieldietzler changed the title refactor(web, server)!: Refactor/search box refactor(web): Refactor/search box Feb 24, 2024
@danieldietzler danieldietzler changed the title refactor(web): Refactor/search box refactor(web): search box Feb 24, 2024
@jrasm91 jrasm91 dismissed their stale review February 24, 2024 23:36

Up for debate

@jrasm91 jrasm91 marked this pull request as draft February 24, 2024 23:50
@michelheusschen
Copy link
Contributor

Thank you for picking this up. I agree with Jason, because it allow us to only fetch the data we need instead of having to update all suggestions. If we switch to managing filters and suggestions in each component instead of in SearchFilterBox, we could achieve this more easily.

I've pushed some changed that do this for the location filters and also fix #7297. Let me know what you think and feel free to make changes or revert.

Comment on lines 22 to 23
$: countryFilter = filter.country;
$: stateFilter = filter.state;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need those?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To prevent unneeded updates when filter is reassigned, but the values of country or state haven't actually changed

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And this prevents this? From my understanding countryFilter updates whenever filter.country updates, so updateStates will update whenever filter.country changes?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this will prevent updating like shown in this demo

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting. TIL!

@danieldietzler
Copy link
Member Author

Thank you for picking this up. I agree with Jason, because it allow us to only fetch the data we need instead of having to update all suggestions. If we switch to managing filters and suggestions in each component instead of in SearchFilterBox, we could achieve this more easily.

I've pushed some changed that do this for the location filters and also fix #7297. Let me know what you think and feel free to make changes or revert.

Thanks, I like this approach much more!

Copy link
Contributor

@mertalev mertalev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Loving these changes!

Comment on lines +19 to +20
$: updateMakes(modelFilter);
$: updateModels(makeFilter);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will have to be updated when #7382 is merged since it's calling an async function without error handling. I see the same in other files below

@alextran1502 alextran1502 merged commit 3e8af16 into main Feb 26, 2024
26 checks passed
@alextran1502 alextran1502 deleted the refactor/search-box branch February 26, 2024 21:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants