-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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 people #9082
Conversation
Deploying immich with
|
Latest commit: |
f4ca564
|
Status: | ✅ Deploy successful! |
Preview URL: | https://0b7f0693.immich.pages.dev |
Branch Preview URL: | https://fix-refactor-search-people.immich.pages.dev |
f459ddd
to
18cfb55
Compare
18cfb55
to
5f80630
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rough first review
searchedPeopleLocal = searchNameLocal(searchName, searchedPeople, numberPeopleToSearch); | ||
}; | ||
|
||
export let handleSearch = async (force?: boolean, name?: string) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does this method accept a name? I don't see any place where that's passed. Actually, I don't even see the point why that method should be exported in the first place
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also personally find this method quite hard to read. I'm not 100% sure why that is; I think it's just many ifs for a method I'd expect to be simple :D
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's because of
onMount(async () => {
const getSearchedPeople = $page.url.searchParams.get(QueryParameter.SEARCHED_PEOPLE);
if (getSearchedPeople) {
searchName = getSearchedPeople;
await handleSearchPeople(true, searchName);
}
});
The search is saved in a query parameter so onMount, if you don't have that handleSearchPeople
, you don't trigger the search and searchName
in the search-people
component is not yet binded to the searchName
from its parent.
The issue with that component is that it's doing the search for every components. It was implemented 4 times with different features and bugs, I prefer to have one which is doing everything well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But handleSearchPeople
is a different method, no? I'm confused...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope, it's binded to handleSearch
like that :
<SearchPeople
type="searchBar"
placeholder="Search people"
onReset={onResetSearchBar}
onSearch={handleSearch}
bind:searchName
bind:searchedPeopleLocal
bind:isSearching={isSearchingPeople}
bind:handleSearch={handleSearchPeople}
/>
so the parent component can interact with the child method.
import { searchNameLocal } from '$lib/utils/person'; | ||
import { searchPerson, type PersonResponseDto } from '@immich/sdk'; | ||
|
||
export let searchName: string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this actually need to be exported?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, so the parent component know what's the user input, it allowed me to simplify that part thanks to that :
$: showPeople = searchName ? searchedPeopleLocal : people.filter((person) => !person.isHidden)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh now it's starting to make sense to me....
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah at the end, parent components "just" get a nice searchbar and a list of suggested people, they can do whatever they want with that list (most of the time filtering) and searchName
is a nice way to know when the user is searching a person or not
use:initInput | ||
<SearchPeople | ||
type="input" | ||
bind:searchName |
There was a problem hiding this 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 you need to bind the name since SearchPeople
won't update it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as before, but I can probably simplify with something like that 15c1e89
26e4009
to
830f8ad
Compare
830f8ad
to
daa24cc
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we rename this component to something more descriptive? Is it a UI element? which type of element is it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
UI + handlers to avoid useless API requests. I renamed it people-search
to follow the same pattern as the other files
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this search bar component only used for people's searches?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, it's used for the album search too
|
||
export let screenHeight: number; | ||
export let people: PersonResponseDto[]; | ||
export let peopleCopy: PersonResponseDto[]; | ||
export let unselectedPeople: PersonResponseDto[]; | ||
|
||
let name = ''; | ||
let searchWord: string; | ||
let isSearchingPeople = false; | ||
let showPeople: PersonResponseDto[]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this showPeople
mean search result?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it's for the people shown to the users.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the improvement is good, however, it is still quite complex to follow the logic of the component around. Is there any way we can think of a fool-proof design for these components so you know exactly what they are doing?
Yes, the logic is complex, we can probably optimize it but we will probably hit some limits. I prefer to have one complex component rather than writing the same component/handler multiple times. |
This PR aims to simplify and reduce bugs with search people. Currently there are multiple implementations for the same search, this PR creates a new modal for all person search.
fix #5571