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: readonly album sharing #8720

Merged
merged 79 commits into from
Apr 25, 2024

Conversation

mgabor3141
Copy link
Contributor

@mgabor3141 mgabor3141 commented Apr 11, 2024

First significant contribution.

This PR adds the ability for album owners to share an album with a specific user without letting them add images to or remove images from the album. It includes database, backend, API as well as frontend changes to achieve this.

Screenshot 2024-04-12 at 19 00 10

Relates to #1607

@mgabor3141 mgabor3141 force-pushed the feature/readonly-sharing branch 3 times, most recently from b0a5c30 to 40a38c7 Compare April 11, 2024 21:43
server/src/controllers/album.controller.ts Outdated Show resolved Hide resolved
server/src/dtos/album.dto.ts Outdated Show resolved Hide resolved
server/src/entities/album-permission.entity.ts Outdated Show resolved Hide resolved
server/src/entities/album-permission.entity.ts Outdated Show resolved Hide resolved
server/src/entities/album.entity.ts Outdated Show resolved Hide resolved
server/src/repositories/album-permission.repository.ts Outdated Show resolved Hide resolved
server/src/repositories/album.repository.ts Outdated Show resolved Hide resolved
server/src/services/album.service.ts Outdated Show resolved Hide resolved
server/src/services/album.service.ts Outdated Show resolved Hide resolved
web/src/routes/(user)/albums/[albumId]/+page.svelte Outdated Show resolved Hide resolved
@mgabor3141 mgabor3141 marked this pull request as ready for review April 12, 2024 17:00
…owners can have access too"

This reverts commit 1343bfa.
@mgabor3141
Copy link
Contributor Author

mgabor3141 commented Apr 24, 2024

Screenshot 2024-04-24 at 13 26 23Screenshot 2024-04-24 at 13 26 30

Played around with the design a bit. I think this is better for accessibility and also for usability as less clicking needs to happen. Thoughts?

I'm not too sure about the wording. Would Editors (plural) look a little better?

@jrasm91
Copy link
Contributor

jrasm91 commented Apr 24, 2024

This PR relies on new API properties that haven't been released yet. Blindly including mobile changes for them would probably break album sharing for most people at the time of the release, so I'd recommend adding them later.

@alextran1502
Copy link
Contributor

Screenshot 2024-04-24 at 13 26 23Screenshot 2024-04-24 at 13 26 30

Played around with the design a bit. I think this is better for accessibility and also for usability as less clicking needs to happen. Thoughts?

I'm not too sure about the wording. Would Editors (plural) look a little better?

I think with this design, you cannot add UserA as Editor and UserB as Viewer in a single operation

@mgabor3141
Copy link
Contributor Author

mgabor3141 commented Apr 24, 2024

I think with this design, you cannot add UserA as Editor and UserB as Viewer in a single operation

Yes, and I like that. Let's say you want to add users as 50/50 editors and viewers. Adding one group first and then the rest in another operation is much fewer clicks than having to click three times for each user in one group before clicking add.

The possibility is also there to just add everyone then change the role of a few users on the other (view users) screen.

But the most common usecase I think is to add all users as either one, which works a lot better here.

@alextran1502
Copy link
Contributor

But the most common usecase I think is to add all users as either one, which works a lot better here.

Yes please, and thanks

@alextran1502
Copy link
Contributor

I think there might be a misunderstanding with my previous comment; I would like to be able to set the permission for users individually, similar to your first UI work

image

@mgabor3141
Copy link
Contributor Author

What do you think about adding a Custom option to the bottom dropdown that would enable the individual dropdowns on each row? That would be the best of both solutions.

Also I won't have too much time to work on this for the rest of this week. I'll see what I can do.

@alextran1502
Copy link
Contributor

What do you think about adding a Custom option to the bottom dropdown that would enable the individual dropdowns on each row? That would be the best of both solutions.

Please don't. I'd prefer a streamlined operation. Introducing many variations is not a good design; too many choices will confuse the common user. I understand that this serves your specific use case; however, when we add a feature, we have to think about the majority of the user group.

@mgabor3141
Copy link
Contributor Author

mgabor3141 commented Apr 24, 2024

I have reverted to the one dropdown per user solution.

I still think there's more that we could do here. There has to be a compromise between clarity and functionality. I'll be very interested to hear what other people who have many users on their instance will think.

E2E test succeeded locally, I'm not sure what this timeout is about. The web test fail is legit, it's due to the svelte warning that I mentioned above. I'm not sure what would be the best way to solve it.

@jrasm91
Copy link
Contributor

jrasm91 commented Apr 24, 2024

image
image
image

@mgabor3141
Copy link
Contributor Author

To add some more references to this topic, this is how Google Docs does its sharing modal:

Screenshot 2024-04-24 at 22 54 45

Here it also takes multiple operations to add users as multiple different roles, but it is very clear in what's going on.

Compared to Google Photos:
Screenshot 2024-04-24 at 22 58 04

Google Photos doesn't have readonly sharing as a concept, at least not from the add screen. It lists all users and one click simply adds them to the shared album. Even if you do link sharing, whoever opens the link has a Join button that they can click to add themselves just as if they were directly invited.

"Readonly" happens in the options panel, where the album owner can toggle adding and removing photos GLOBALLY for all joined users (they call it "collaborate"):

Screenshot 2024-04-24 at 23 03 57

I think the pattern I have in mind is a really effective one. People who are present on a trip or at an event can all join an album as Editors and upload all their photos. They can then add whoever they want as Viewers. The really cool thing is that everyone's Albums page will be populated with all the various albums they have access to (as any role). This is something that link sharing doesn't really achieve.

This pattern depends on being able to add many viewers easily. This version with three clicks per viewer user per album is I think a non-starter for many users who would like to adopt a similar use pattern with regards to sharing.

@mgabor3141
Copy link
Contributor Author

@jrasm91 I really like what you did with the modal. I think this one should be the "options/edit users" modal, and there could be a separate add users modal that's more closer to the Google Docs style interface.

That's all I wanted to say, whatever we go with is going to be better than it was before. Thanks for reading me ramble about it.

Copy link
Contributor

@alextran1502 alextran1502 left a comment

Choose a reason for hiding this comment

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

Thank you very much! I know there were a lot of back and forth about the UI. I am open for additional user feedback, and we will make changes if need

@alextran1502 alextran1502 enabled auto-merge (squash) April 25, 2024 04:17
@alextran1502 alextran1502 merged commit 2943f93 into immich-app:main Apr 25, 2024
22 checks passed
@mgabor3141 mgabor3141 deleted the feature/readonly-sharing branch April 25, 2024 05:43
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

4 participants