-
Notifications
You must be signed in to change notification settings - Fork 2k
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
feat(web,server): search people #5703
Conversation
Deploying with
|
Latest commit: |
6d38cf3
|
Status: | ✅ Deploy successful! |
Preview URL: | https://48c5212e.immich.pages.dev |
Branch Preview URL: | https://feat-search-name-people-page.immich.pages.dev |
48b2257
to
6d71391
Compare
6d71391
to
09013e2
Compare
When you're on the people page (web) and rename a person, then it seems like the original people and peopleCopy get out of sync. Steps:
Effect: name change not visible until you delete the search string |
|
||
$: { | ||
if (searchName) { | ||
people = searchNameLocal(searchName, searchPeopleCopy, 10); |
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.
people = searchNameLocal(searchName, searchPeopleCopy, 10); | |
people = searchNameLocal(searchName, searchPeopleCopy, 100); |
The result set is limited to 10. Can you raise that to...let's say 100?
I think this is more appropriate for the big people list.
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 increased it to 20; the API to search people returns only 20 people
Rather that a tooltip showing the number, it feels like the info would be more visible if you wrote "People (77)" and updated the qty to match the number of people in the list? With the current implementation, you really need to know it's there to find 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.
There are quite a few changes on the web side of things, and I am not familiar enough with the person/face/merge code to know exactly what is going on.
In the future breaking some of the unrelated changes into a smaller, separate PR would be helpful.
The server changes seem fine as far as I can tell as well. I haven't tested the changes, but I assume that you have 😄
@@ -59,6 +60,32 @@ export const dateFormats = { | |||
}, | |||
}; | |||
|
|||
export enum QueryParameter { |
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.
In the future, this would be a good change to do in a separate pull request. If you find yourself refactoring stuff in order to make the current task easier, considering doing it in a separate pull request, as it is easier to review, test, etc. (easier to review means it gets merged faster 😄)
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.
Refactoring is too good 😜
All joking aside, yes I should keep separate changes in separate PRs, but every time I start refactoring something I'm like "it's a small change, it's not worth a whole PR for so little change"
For this case, I limited myself to "only" change the way query parameters are handled. I plan to refactor the routing through the webapp.
I'm a little late, but maybe you can make the search something that's just an icon and when you click on it it opens to a search box? I think it's more aesthetic. In any case, this is a great addition, thank you 0: |
Changes made in this PR
This PR adds an option to search people on the /people page. It adds a title on
People
to know how many people the user has on its librariesScreenshots
2023-12-14.22-35-04.mp4