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

fix(web): cancel people merge selection: do not show "Change name successfully" notification #15744

Merged

Conversation

afv
Copy link
Contributor

@afv afv commented Jan 28, 2025

  • Change a person name and select one of the existing;
  • "Are these the same person?" modal appears;
  • Cancel; the "Change name successfully" notification appears (without the name being changed).

This PR fixes the notification showing if the user cancels the merge.

@alextran1502
Copy link
Contributor

I believe the correct fix is to consolidate the onClose and onReject into a single callback i.e onClose in the merge-suggestion-model.svelte component

image

@afv
Copy link
Contributor Author

afv commented Jan 28, 2025

We have to distinguish between

  1. "Cancel, I clicked the wrong person" when explicitly clicking an existing name (button onclick={() => handleSuggestPeople(person)}), and
  2. "Cancel, the suggested person that exists with the same name is not the same person, so keep/continue the name change", when the modal appears when you give a name that already exists.

Will your suggestion of consolidating the onClose and onReject work with the two cases?

@alextran1502
Copy link
Contributor

The onReject callback from the merge-suggestion-model.svelte is called when you click the No button, essentially having the same effect as closing the model. I don't see the difference there. So I think it should just be handled as an onClose event

@alextran1502
Copy link
Contributor

Ah wait, nvm, I get what you mean. Your solution is fine

@alextran1502 alextran1502 merged commit 060300d into immich-app:main Jan 28, 2025
34 of 35 checks passed
@afv
Copy link
Contributor Author

afv commented Jan 28, 2025

handleNameChange opens the modal if you enter a name that already exists:

image

If you say No and onReject just closes the modal, the name won't be saved.

@afv
Copy link
Contributor Author

afv commented Jan 28, 2025

Ah wait, nvm, I get what you mean. Your solution is fine

Thanks!

@afv afv deleted the fix-people-merge-cancel-notification branch January 28, 2025 17:54
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