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

feat(web): better UX when creating a new album #8270

Merged
merged 6 commits into from Mar 27, 2024

Conversation

Ethan13310
Copy link
Contributor

@Ethan13310 Ethan13310 commented Mar 25, 2024

This PR fixes something if find annoying:

Imagine you are sorting your thousands of photos into albums. You select Add to... > Album, then using the modal, you create a new album. You will be redirected to this album without being prompted. Now you have to go back and find yourself at the top of the "Photos" page, and have scroll again thousands of photos to where you previously were.

To fix this, a notification is now sent when adding assets to a new album, with a "View Album" button:

ViewAlbum

I also updated the search function to ignore accents:

SearchAccents

@jrasm91
Copy link
Contributor

jrasm91 commented Mar 25, 2024

The way this pattern works in google photos is a notification is shown, which has a button/link you can click to "View X". Our actions already have the ability to perform and action when they are clicked, so maybe that would be a better implementation.

@Ethan13310
Copy link
Contributor Author

Ethan13310 commented Mar 26, 2024

The way this pattern works in google photos is a notification is shown, which has a button/link you can click to "View X". Our actions already have the ability to perform and action when they are clicked, so maybe that would be a better implementation.

That would indeed be way better, less intrusive. However it seems notification cards cannot display a button (unlike Google Photos' ones), they can only be clickable, without any obvious hint to the user. I'll add a way to display a button then.

@Ethan13310 Ethan13310 marked this pull request as draft March 26, 2024 10:16
@waclaw66
Copy link
Contributor

It would be nice to se new album name instead of "to new album" in the toast as well.

Added 11 assets into My new album
[Show album]

@Ethan13310
Copy link
Contributor Author

I edited the PR description. The notification card now looks like this when a button is added:

NotifCardButtons

To do this:

notificationController.show({
    // ...
    action: {
      type: 'link',
      button: 'Button Text',
      target: '/route/stuff'
    }
  });

If the button property is omitted, then the notification is clickable like it was before.

@Ethan13310
Copy link
Contributor Author

It would be nice to se new album name instead of "to new album" in the toast as well.

Added 11 assets into My new album [Show album]

Like this?

NotifWithAlbumName

@jrasm91
Copy link
Contributor

jrasm91 commented Mar 26, 2024

Looks great but can it be View Album instead?

@Ethan13310
Copy link
Contributor Author

Looks great but can it be View Album instead?

Yep, done!

@Ethan13310 Ethan13310 marked this pull request as ready for review March 26, 2024 13:31
Comment on lines +40 to +44
button: {
text: 'View Album',
onClick() {
return goto(`${AppRoute.ALBUMS}/${albumId}`);
},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! This looks pretty clean to me. Although you could also do:

onClick: () => goto(...)

Copy link
Contributor

@jrasm91 jrasm91 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I can try to test it out later unless someone picks it up first.

@alextran1502 alextran1502 merged commit 8bf571b into immich-app:main Mar 27, 2024
23 checks passed
@Ethan13310 Ethan13310 deleted the feat/goto-album-confirm branch March 27, 2024 21:56
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.

None yet

5 participants