-
-
Notifications
You must be signed in to change notification settings - Fork 4.5k
feat(web): add open in new tab and explicit merge buttons to people thumbnails in merge and reassign pages #23648
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
base: main
Are you sure you want to change the base?
Conversation
|
Label error. Requires exactly 1 of: changelog:.*. Found: 🖥️web. A maintainer will add the required label. |
bb220d6 to
452191b
Compare
|
Rebased with v2.3.0 and added keyboard navigation and support for touch devices (buttons always visible) |
danieldietzler
left a comment
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.
Code-wise this seems fine to me. @alextran1502 your call if you want this UX
|
|
||
| {#if selectable} | ||
| <div | ||
| class="absolute left-1/2 -bottom-6 flex -translate-x-1/2 gap-2 opacity-0 transition-opacity group-hover:opacity-100 group-focus-within:opacity-100 [@media(hover:none)]:opacity-100" |
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 you could simplify this to hidden hover:visible, right?
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.
This would remove the fade in / fade out animation, but fine by me either way. Let me know what you prefer
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 I think that's fine, we don't need that
…humbnails in merge and reassign pages
452191b to
ea0a7ba
Compare
Description
When merging people, clicking a person immediately marks them for merging. Instead, this PR adds two icon buttons (shown on hover) to allow:
This addresses a personal issue when merging people with large year gaps or blurry featured photos, where I feel the need to verify the person's other photos before confidently merging them.
The blue tint on hover was removed and the selected prop was repurposed for controlling whether the plus or minus button appears.
How Has This Been Tested?
Screenshots (if appropriate)
Checklist:
src/services/uses repositories implementations for database calls, filesystem operations, etc.src/repositories/is pretty basic/simple and does not have any immich specific logic (that belongs insrc/services/)Please describe to which degree, if any, an LLM was used in creating this pull request.
Code and project discovery in general, brainstorming, elaborating the PR description.