Skip to content

Conversation

@artonge
Copy link
Collaborator

@artonge artonge commented May 11, 2023

Revert part of #1781 to keep unique album names, and improve rendering of cover and header of shared albums

Based on concerns expressed here: #1781 (comment)

Before After
Screenshot from 2023-05-11 14-39-46 Screenshot from 2023-05-11 14-34-08
Screenshot from 2023-05-11 14-39-32 Screenshot from 2023-05-11 14-34-15

@artonge artonge enabled auto-merge May 11, 2023 09:19
@artonge artonge self-assigned this May 11, 2023
@artonge artonge added enhancement New feature or request 3. to review Waiting for reviews php PHP related ticket javascript Javascript related ticket labels May 11, 2023
@artonge artonge added this to the Nextcloud 27 milestone May 11, 2023
@artonge artonge force-pushed the artonge/fix/revert_display_name_in_shared_album branch from b4827ad to 3a36fd4 Compare May 11, 2023 09:26
@artonge
Copy link
Collaborator Author

artonge commented May 11, 2023

/backport to stable26

@artonge
Copy link
Collaborator Author

artonge commented May 11, 2023

/backport to stable25

@backportbot-nextcloud backportbot-nextcloud bot added the backport-request Pending backport by the backport-bot label May 11, 2023
@nimishavijay
Copy link
Member

nimishavijay commented May 11, 2023

This looks great! Since this PR purpose is to make it clear who shared the item with you, I would suggest showing the full name along with the avatar, using a user bubble (without the primary color), screenshots attached. What do you think? cc @szaimen :)

@szaimen
Copy link
Contributor

szaimen commented May 11, 2023

Yes, using a userbubble including the display name is a good idea. The mockup looks good to me! :)

@artonge artonge force-pushed the artonge/fix/revert_display_name_in_shared_album branch from 3a36fd4 to c6bcc03 Compare May 11, 2023 12:38
@artonge
Copy link
Collaborator Author

artonge commented May 11, 2023

Thanks, screenshots updated. Good to review :)

Copy link
Contributor

@szaimen szaimen left a comment

Choose a reason for hiding this comment

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

Screenshots look good to me :)

Copy link
Member

@nimishavijay nimishavijay left a comment

Choose a reason for hiding this comment

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

🚀 :)

@artonge artonge force-pushed the artonge/fix/revert_display_name_in_shared_album branch from c6bcc03 to 1c095ea Compare May 11, 2023 14:27
Copy link
Member

@pulsejet pulsejet left a comment

Choose a reason for hiding this comment

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

Love that bubble 💯

@artonge artonge force-pushed the artonge/fix/revert_display_name_in_shared_album branch from 1c095ea to 2144dd2 Compare May 12, 2023 10:50
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.

Awesome, looks much better with the userbubble.

Detail: In the grid list, the "Shared by" could go on the same line as date and "X items" as that's not so long @artonge? Then there is less of a vertical change between shared and non-shared albums.

@artonge
Copy link
Collaborator Author

artonge commented May 15, 2023

Detail: In the grid list, the "Shared by" could go on the same line as date and "X items" as that's not so long @artonge? Then there is less of a vertical change between shared and non-shared albums.

I am worried that this would overflow in most cases, so keeping it as is.

@artonge artonge force-pushed the artonge/fix/revert_display_name_in_shared_album branch from 2144dd2 to 67c9374 Compare May 15, 2023 15:13
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.

Right, didn’t take into account the names itself. Approving then :)

@artonge artonge force-pushed the artonge/fix/revert_display_name_in_shared_album branch from 67c9374 to 5e420e9 Compare May 15, 2023 20:42
+ Improve rendering of cover and header of shared albums

Signed-off-by: Louis Chemineau <[email protected]>
@artonge artonge force-pushed the artonge/fix/revert_display_name_in_shared_album branch from 5e420e9 to 8c71a53 Compare May 16, 2023 07:54
@artonge artonge merged commit 4f7e63e into master May 16, 2023
@artonge artonge deleted the artonge/fix/revert_display_name_in_shared_album branch May 16, 2023 08:04
@backportbot-nextcloud
Copy link

The backport to stable26 failed. Please do this backport manually.

# Switch to the target branch and update it
git checkout stable26
git pull origin/stable26

# Create the new backport branch
git checkout -b fix/foo-stable26

# Cherry pick the change from the commit sha1 of the change against the default branch
# This might cause conflicts. Resolve them.
git cherry-pick abc123

# Push the cherry pick commit to the remote repository and open a pull request
git push origin fix/foo-stable26

More info at https://docs.nextcloud.com/server/latest/developer_manual/getting_started/development_process.html#manual-backport

@backportbot-nextcloud
Copy link

The backport to stable25 failed. Please do this backport manually.

# Switch to the target branch and update it
git checkout stable25
git pull origin/stable25

# Create the new backport branch
git checkout -b fix/foo-stable25

# Cherry pick the change from the commit sha1 of the change against the default branch
# This might cause conflicts. Resolve them.
git cherry-pick abc123

# Push the cherry pick commit to the remote repository and open a pull request
git push origin fix/foo-stable25

More info at https://docs.nextcloud.com/server/latest/developer_manual/getting_started/development_process.html#manual-backport

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

3. to review Waiting for reviews backport-request Pending backport by the backport-bot enhancement New feature or request javascript Javascript related ticket php PHP related ticket

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants