-
-
Notifications
You must be signed in to change notification settings - Fork 3.8k
feat(web): Add to Multiple Albums #20072
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?
feat(web): Add to Multiple Albums #20072
Conversation
- update modal for multi select - Update add-to-album and add-to-album-action to work with new array return from AlbumPickerModal - Add asset-utils.addAssetsToAlbums (incomplete)
- add test
- make open-api
- handle notification
- clean up
- format & check
- fix assets_cannot_be_added language call
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.
Thanks for the PR!
Appreciate the review! Adding some of your points to my own PR checklist lol. Aiming to have cleaner PRs 👍 |
- don't nest the buttons
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 is starting to look pretty good. I still don't like the error handling/response stuff but I guess that's a bigger discussion to have.
@@ -511,6 +515,7 @@ | |||
"assets_trashed_count": "Trashed {count, plural, one {# asset} other {# assets}}", | |||
"assets_trashed_from_server": "{count} asset(s) trashed from the Immich server", | |||
"assets_were_part_of_album_count": "{count, plural, one {Asset was} other {Assets were}} already part of the album", | |||
"assets_were_part_of_albums_count": "{count, plural, one {Asset was} other {Assets were}} already part of the albums", |
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.
Have you just... duplicated this key from the line above? 👀
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.
album -> albums lol
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.
Ah damn it I'm blind 😅
} else if (results.error && res.error !== BulkIdErrorReason.DUPLICATE) { | ||
results.error = BulkIdErrorReason.UNKNOWN; | ||
} |
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.
Do you need to results.success = false
here too?
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 don't believe so. results.success is initialized false, and if one album succeeds it should be permanently true.
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.
Ah got it
- apply new api permission
📖 Documentation deployed to pr-20072.preview.immich.app |
Are you leaning towards more verbose responses, or less? Whole thing could probably be cleaned up a bit if it was a simple pass/fail, with no extra error messages if any albums failed. |
I'm leaning towards something much simpler, less verbose, but I'd be interested in Jason's take on this. |
Description
Added ability to select multiple albums from the AlbumPickerModal. Hovering over an album shows a checkbox (similar to Thumbnail), and once an album is 'multiSelected', clicking anywhere on an album checks and unchecks it. Finalize choice by clicking the submit button, or pressing Enter (pressing 'm' or long pressing on a selected album will also multi-select it.
Notifications when attempting to add to multiple albums are simplified, with no links to the albums. Instead, counts of how many assets were added to how many albums are displayed. ex. "1 asset added to 3 albums".
A new endpoint is added (PUT /albums/assets), with a new body dto and new return dto with the final counts. If any asset is successfully added to any album, the return is a success, with total counts adjusted to announce how many assets were added to albums.
How Has This Been Tested?
Screenshots (if appropriate)
API Changes
Added PUT /albums/assets
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/
)