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

Add collaborators management views #1161

Merged
merged 1 commit into from
Sep 16, 2022
Merged

Conversation

artonge
Copy link
Collaborator

@artonge artonge commented Aug 24, 2022

Also contains some minor css tweaks.

Screenshots

Collaborator list With some selection Without public link during creation
Screenshot from 2022-08-25 12-46-44 Screenshot from 2022-08-25 12-46-53 Screenshot from 2022-08-25 13-30-38
Initial loading Loading when searching
Screenshot from 2022-08-25 12-46-42 Screenshot from 2022-08-25 12-47-04
Public link created Public link copied
Screenshot from 2022-08-25 12-46-16 Screenshot from 2022-08-25 12-46-18

Fix #1226

@artonge artonge self-assigned this Aug 24, 2022
@artonge artonge added 3. to review Waiting for reviews javascript Javascript related ticket feature: albums Related to the albums section labels Aug 25, 2022
@artonge artonge requested a review from jancborchardt August 25, 2022 11:53
@artonge artonge force-pushed the feat/album_collaborators branch from a032535 to 166d387 Compare August 25, 2022 11:57
Copy link
Member

@jancborchardt jancborchardt left a comment

Choose a reason for hiding this comment

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

Looks good, just a few things:

  • The "Share link" finction looks oddly placed and not interactive. Maybe better as a secondary button on the bottom left instead?
  • The gab below the input field looks off. Wouldn't this work like in Files, where people are picked one by one from the suggestion dropdown?
  • Possibly a general Vue component thing, but there should be a close x within the modal on the top right
  • Wording in the title subline: "users" to "people", and " group" needs to be plural "groups"

@artonge artonge changed the title All collaborators management views Add collaborators management views Aug 30, 2022
@artonge artonge force-pushed the feat/album_collaborators branch 2 times, most recently from d0f6efb to bcf36f1 Compare August 31, 2022 13:58
@nimishavijay
Copy link
Member

nimishavijay commented Aug 31, 2022

Looks very nice! Agree with everything mentioned, plus a few more points:

  • "Save collaborators list" --> "Save" or maybe "Save collaborators"
  • Usually the icon associated with "Save" is the checkmark icon. The paper plane icon is usually for send. I'm also wondering if we need an icon at all
  • "Share this album via link" --> "Share via public link"? To make it clearer that it's a public link and to make it shorter if it's going to be a button on the bottom left
  • We can also add an icon to the "Share via public link" button as when it's clicked it changes into the "Copy link" button which has an icon
  • Maybe there can also be a way to unshare in case the "Share via public link" button was clicked by accident. If we are going with the secondary button on the bottom left, then there could possibly be:
    • a trash icon or close icon next to it on the right which would unshare
    • or a 3 dot menu with one item "Unshare"

cc @jancborchardt as well

@artonge artonge force-pushed the feat/album_collaborators branch 6 times, most recently from 0a1f2c5 to 7d3b378 Compare September 7, 2022 08:51
@skjnldsv skjnldsv added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Sep 8, 2022
@artonge artonge force-pushed the feat/album_collaborators branch 2 times, most recently from ad6126a to 13ad35c Compare September 12, 2022 13:55
@skjnldsv

This comment was marked as resolved.

@artonge artonge force-pushed the feat/album_collaborators branch from 48d9e0e to f24e28b Compare September 14, 2022 16:11
lib/Album/AlbumMapper.php Outdated Show resolved Hide resolved
lib/Album/AlbumMapper.php Outdated Show resolved Hide resolved
lib/Album/AlbumMapper.php Show resolved Hide resolved
lib/Controller/PreviewController.php Show resolved Hide resolved
lib/Migration/Version20001Date20220830131446.php Outdated Show resolved Hide resolved
src/mixins/FetchAlbumsMixin.js Outdated Show resolved Hide resolved
src/router/index.js Outdated Show resolved Hide resolved
src/services/PhotoSearch.js Outdated Show resolved Hide resolved
src/views/Timeline.vue Show resolved Hide resolved
src/store/files.js Show resolved Hide resolved
@skjnldsv skjnldsv added this to the Nextcloud 25 milestone Sep 15, 2022
@artonge artonge force-pushed the feat/album_collaborators branch 2 times, most recently from b1b4090 to 5fcd3f9 Compare September 15, 2022 15:28
lib/Album/AlbumMapper.php Show resolved Hide resolved
lib/Album/AlbumMapper.php Show resolved Hide resolved
lib/Album/AlbumMapper.php Show resolved Hide resolved
lib/Album/AlbumMapper.php Show resolved Hide resolved
lib/Album/AlbumMapper.php Show resolved Hide resolved
lib/Album/AlbumMapper.php Show resolved Hide resolved
lib/Sabre/Album/AlbumPhoto.php Show resolved Hide resolved
lib/Sabre/Album/AlbumPhoto.php Show resolved Hide resolved
lib/Sabre/Album/AlbumPhoto.php Show resolved Hide resolved
Copy link
Member

@PVince81 PVince81 left a comment

Choose a reason for hiding this comment

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

Reviewed together, looks good 👍

the only critical part I think is the DB schema to remove the cached display name and use the display name cache instead / lazy user

@PVince81
Copy link
Member

for retrieving display names, use $this->userManager->getDisplayName($userId) to make sure it hits the cache

@artonge
Copy link
Collaborator Author

artonge commented Sep 16, 2022

/compile amend /

Signed-off-by: Louis Chemineau <louis@chmn.me>
Signed-off-by: nextcloud-command <nextcloud-command@users.noreply.github.com>
@artonge artonge merged commit ae0379c into master Sep 16, 2022
@artonge artonge deleted the feat/album_collaborators branch September 16, 2022 07:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2. developing Work in progress feature: albums Related to the albums section javascript Javascript related ticket
Projects
None yet
Development

Successfully merging this pull request may close these issues.

500 error if you remove a file belonging to an album
5 participants