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

Rework tags view #1191

Merged
merged 17 commits into from
Sep 1, 2022
Merged

Rework tags view #1191

merged 17 commits into from
Sep 1, 2022

Conversation

marcelklehr
Copy link
Member

@marcelklehr marcelklehr commented Aug 30, 2022

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.

Wow so cool! Looks really nice :) Some details:

  • The title in the navigation would be better as "Tags" instead of "Tagged photos"?
  • The popular tags are not sorted by ones with most photos yet
  • Some of the previews seem to have square corners instead of rounded, possibly cause they are not filling the container, not sure?
  • The titles "Popular tags" and "All tags" could be moved a bit to the right to left-align with the leftmost image preview (not the hover effect)
  • Between the "Popular tags" section and the "All tags" heading, a bit of whitespace would be nice (similar to how we do it in the "All media" view for the month headings)
  • The albums look quite large, or is that just me? Maybe it’s fine. :) Is the size fixed, or calculated based on screen size? (cc @artonge as I guess it just uses the album size and we should make sure the sizing is the same :)

Otherwise all really really nice!

@artonge
Copy link
Collaborator

artonge commented Aug 31, 2022

[ ] The albums look quite large, or is that just me? Maybe it’s fine. :) Is the size fixed, or calculated based on screen size? (cc @artonge as I guess it just uses the album size and we should make sure the sizing is the same :)

Not the same component, but I assume @marcelklehr used the same size.

@nimishavijay
Copy link
Member

Woah looks super cool! :) Agree with everything, especially the sizing of the photos, plus possibly:

  • when you are viewing the photos of a tag, show the number of photos as a subline to the title of the tag

@marcelklehr
Copy link
Member Author

The albums look quite large, or is that just me?

Tag cover size is the same as album covers, but will get smaller on mobile.

The popular tags are not sorted by ones with most photos yet

Done.

@marcelklehr
Copy link
Member Author

marcelklehr commented Sep 1, 2022

New screens:

image

https://cloud.marcelklehr.de/s/QKZ2AxfSdqpjzXG

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 great, good to go design-wise! :)

@marcelklehr marcelklehr marked this pull request as ready for review September 1, 2022 10:55
Copy link
Collaborator

@artonge artonge 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.

Doesn't it miss some padding in the TagContent. Is it en purpose ?

src/components/TagCover.vue Outdated Show resolved Hide resolved
@marcelklehr
Copy link
Member Author

marcelklehr commented Sep 1, 2022

Doesn't it miss some padding in the TagContent. Is it en purpose ?

I didn't add any padding after reworking, yeah. The timeline view for example indeed has padding. Do we want padding on the photos container here @nimishavijay @jancborchardt?

Signed-off-by: Marcel Klehr <[email protected]>
Signed-off-by: Marcel Klehr <[email protected]>
Signed-off-by: Marcel Klehr <[email protected]>
Signed-off-by: Marcel Klehr <[email protected]>
Signed-off-by: Marcel Klehr <[email protected]>
Co-authored-by: Louis <[email protected]>
Signed-off-by: Marcel Klehr <[email protected]>
@marcelklehr
Copy link
Member Author

/compile amend /js

@jancborchardt
Copy link
Member

I didn't add any padding after reworking, yeah. The timeline view for example indeed has padding. Do we want padding on the photos container here @nimishavijay @jancborchardt?

Right, yes would be good if it has the same padding as on the timeline view. :)

@marcelklehr
Copy link
Member Author

/compile amend /js

Signed-off-by: Marcel Klehr <[email protected]>
Signed-off-by: nextcloud-command <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants