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(ui): add 'show more/less...' for studios on movie details page #1770

Merged
merged 3 commits into from
Jul 13, 2021
Merged

feat(ui): add 'show more/less...' for studios on movie details page #1770

merged 3 commits into from
Jul 13, 2021

Conversation

dphildebrandt
Copy link
Contributor

@dphildebrandt dphildebrandt commented Jun 10, 2021

Description

Display a max of 3 studios before prompting to expand the list

Screenshot (if UI-related)

studios

To-Dos

  • Successful build yarn build
  • Translation keys yarn i18n:extract

Issues Fixed or Closed

@TheCatLady
Copy link
Collaborator

TheCatLady commented Jun 10, 2021

We should never use ... instead of the ellipsis character, but I don't really think this looks great as text. Perhaps an icon would look better? Solid variants of chevron-double-down/chevron-double-up maybe? Or the dots-horizontal icon? 🤔

@sct
Copy link
Owner

sct commented Jun 10, 2021

We should never use ... instead of the ellipsis character, but I don't really think this looks great as text. Perhaps an icon would look better? Solid variants of chevron-double-down/chevron-double-up maybe? Or the dots-horizontal icon? 🤔

Chevron would be better for this I feel. We should also change the color to indicate the user can interact with this and its just not another studio?

@TheCatLady
Copy link
Collaborator

Chevron would be better for this I feel. We should also change the color to indicate the user can interact with this and its just not another studio?

Well, FWIW users can interact with the studios in the list as well 😛

@sct
Copy link
Owner

sct commented Jun 10, 2021

Chevron would be better for this I feel. We should also change the color to indicate the user can interact with this and its just not another studio?

Well, FWIW users can interact with the studios in the list as well 😛

True, the icon may be enough to differentiate it.

@dphildebrandt
Copy link
Contributor Author

Updated screenshot in PR body.

@TheCatLady
Copy link
Collaborator

@dphildebrandt You don't need to squash your commits down btw, we squash before merging anyway 😄 Makes it hard to tell what changed when you do that.

@dphildebrandt
Copy link
Contributor Author

@dphildebrandt You don't need to squash your commits down btw, we squash before merging anyway 😄 Makes it hard to tell what changed when you do that.

Ah, I wasn't sure. I was trying to keep the commits atomic, and didn't think some of these changes warranted a full-blown husky-compatible commit message.

@TheCatLady
Copy link
Collaborator

Ah, I wasn't sure. I was trying to keep the commits atomic, and didn't think some of these changes warranted a full-blown husky-compatible commit message.

I think that it makes it harder to difficult to review and see what changed since the last time we viewed the PR. Best to just add separate commits, especially once you've opened a PR and people have begun reviewing it.

Copy link
Owner

@sct sct left a comment

Choose a reason for hiding this comment

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

Sorry for the long wait! Things have been super busy!

Looks great! Thanks for the contribution again!

@sct sct merged commit 680ea0c into sct:develop Jul 13, 2021
@dphildebrandt dphildebrandt deleted the studio-polish branch July 13, 2021 15:27
nicospz pushed a commit to nicospz/overseerr that referenced this pull request Aug 9, 2021
@github-actions
Copy link

🎉 This PR is included in version 1.26.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@Magikarplvl4

This comment has been minimized.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Small enhancement for the studio list on detail page
4 participants