Skip to content

Conversation

@luisdralves
Copy link

@luisdralves luisdralves commented Nov 6, 2025

Description

When merging people, clicking a person immediately marks them for merging. Instead, this PR adds two icon buttons (shown on hover) to allow:

  • Opening the person in a new tab to verify before merging
  • Explicitly adding/removing from merge selection

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?

  • Open a person's page and select "merge people". Hovering a person's face should reveal the 2 action buttons which should behave as described.
  • Select a person for merging. Their thumbnail should appear next to the target person and the action buttons should appear on hover, with a minus for deselecting instead of the plus
  • Open a person's page, select couple of photos, and hit "fix incorrect match". This interface should behave as above.

Screenshots (if appropriate)

Screenshot_20251105_235358 Screenshot_20251105_235436
Screenshot_20251106_000139 Screenshot_20251106_000206

Checklist:

  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation if applicable
  • I have no unrelated changes in the PR.
  • I have confirmed that any new dependencies are strictly necessary.
  • I have written tests for new code (if applicable)
  • I have followed naming conventions/patterns in the surrounding code
  • All code in src/services/ uses repositories implementations for database calls, filesystem operations, etc.
  • All code in src/repositories/ is pretty basic/simple and does not have any immich specific logic (that belongs in src/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.

@immich-push-o-matic
Copy link

immich-push-o-matic bot commented Nov 6, 2025

Label error. Requires exactly 1 of: changelog:.*. Found: 🖥️web. A maintainer will add the required label.

@luisdralves luisdralves force-pushed the feature/people-merge-open-in-new-tab branch from bb220d6 to 452191b Compare November 19, 2025 22:46
@luisdralves
Copy link
Author

Rebased with v2.3.0 and added keyboard navigation and support for touch devices (buttons always visible)

Copy link
Member

@danieldietzler danieldietzler left a 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"
Copy link
Member

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?

Copy link
Author

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

Copy link
Member

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

@luisdralves luisdralves force-pushed the feature/people-merge-open-in-new-tab branch from 452191b to ea0a7ba Compare December 9, 2025 11:42
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.

2 participants