-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Added artist list concatenation for albums that have over 10 artists #4830
Conversation
Please review https://jellyfin.org/docs/general/contributing/development#pull-request-guidelines Pull-request titles will eventually end up in the release change-log, so make sure it's short and descriptive 😺 |
661c37e
to
937aa81
Compare
Kudos, SonarCloud Quality Gate passed! 0 Bugs No Coverage information |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code looks good now. It would be nice if someone could confirm the functionality works as expected... I don't seem to have any albums available that have enough artists to test. 😅
BeforeAfterTV modeMobileExperimentalThe optimal number of entries seem to be highly dependent on resolution, display mode and potentially theme. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should fix the extra slash showing up before the "and other artists" text.
Kudos, SonarCloud Quality Gate passed! 0 Bugs No Coverage information |
Quality Gate passedKudos, no new issues were introduced! 0 New issues |
I don't think this PR solves the issue. It's a good feature to have if you do have more than 10 album artists but not if it's a compilation. |
Quality Gate passedIssues Measures |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of updating the number of artists shown to a hard-coded value, can we instead just use CSS to limit the displayed artists to 2 lines? display: -webkit-box;
-webkit-line-clamp: 2;
-webkit-box-orient: vertical;
overflow: hidden; |
Yes that is correct |
Fantastic then I am going to commit those changes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm pretty sure I accepted these changes...
Please retry analysis of this Pull-Request directly on SonarCloud |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess I missed these at some point?...
@thornbill Any idea as to why I can't get rid of this message on the PR? I already committed the changes and did so a while ago. At least I thought I did. |
Co-authored-by: Bill Thornton <thornbill@users.noreply.github.com>
Co-authored-by: Bill Thornton <thornbill@users.noreply.github.com>
Co-authored-by: felix920506 <felix920506@gmail.com>
Co-authored-by: felix920506 <felix920506@gmail.com>
21e7f3b
to
1a6a442
Compare
Quality Gate passedIssues Measures |
Cloudflare Pages deployment
|
Changes
Added condition to determine if the artist count of music is over 10 and if so, the displaying text will be changed to only display the first 10 artists and will have "and X other artists." concatenated to the end of it.
Issues
Fixes #4228