-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Item Card component displays Critic|Community ratings from setting #6919
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
base: master
Are you sure you want to change the base?
Item Card component displays Critic|Community ratings from setting #6919
Conversation
Cloudflare Pages deployment
|
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.
ESLint doesn't pass. Please fix all ESLint issues.
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.
ESLint doesn't pass. Please fix all ESLint issues.
@@ -321,6 +321,19 @@ export class UserSettings { | |||
return this.get('customCss', false); | |||
} | |||
|
|||
/** |
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 know /scripts
are marked as deprecated, but from what I can tell updating this file is required when adding a display setting.
I'm open to different approaches but I think its important to have a setting. Both to allow for default behavior of still not showing card ratings, and the extra options I added.
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.
It's adding new files that would be an issue, so it's fine.
<h2 class="sectionTitle"> | ||
${ItemCard} | ||
</h2> |
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 did not think any of the other sections fit well enough. Seeing "Item Details" existed I thought "Item Card" was a consistently themed section.
I think "Item Card" is distinct enough and a broad enough design space to justify its own section.
@thornbill I see you're very active in this repo, could you take a look at this PR? Would also appreciate if you could add Thanks |
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.
ESLint doesn't pass. Please fix all ESLint issues.
This pull request has merge conflicts. Please resolve the conflicts so the PR can be successfully reviewed and merged. |
|
@viown I addressed your comments, could you approve this? I'd really like to get it in. |
added Item Card component displays Critic|Community ratings from setting jellyfin#6919
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.
Sorry for the delay, although do note that it can take a while to get reviews and PRs in due to the low number of contributors on the web team (as denoted by the backlog of PRs at the moment 😅).
I personally think that that the setting can just be a simple checkbox to show none or all, but maybe someone else can chip in on that.
return this.set('cardRatings', val.toString(), false); | ||
} | ||
|
||
return this.get('cardRatings', false); |
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.
return this.get('cardRatings', false); | |
return this.get('cardRatings', false) || 'none'; |
|
||
.cardRating { | ||
display: flex; | ||
align-items: center; |
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.
align-items: center; | |
align-items: center; | |
color: #fff; |
The inherited color can make the ratings text hard to read on lighter themes
const backgroundImageClass = item.CriticRating >= 60 ? 'cardRatingFresh' : 'cardRatingRotten'; | ||
cardRatingHtml += `<div class="cardRating"><span class="cardRatingIcon cardCriticRating ${backgroundImageClass}"></span><span>${item.CriticRating}</span></div>`; | ||
} | ||
if (item.CommunityRating && ['community', 'all'].includes(ratingsSetting)) { |
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 think we can put community ratings first? Same order as in the item details page.
@viown yeah no worries, I mostly wanted to make sure it didn't fall through the cracks! I appreciate you leaving feedback, I'll be more patient |
This is a nice feature, and I can absolutely see users wanting the option to display just one or the other. |
Could this be added to the server so clients can take full advantage of this new feature? |
@Hello-World-Traveler I'm not sure what you're asking. This is fully a frontend/client side feature that will work as long as the server is passing critic/community rating data. If you had something else in mind I didn't address let me know, thanks |
@shoecar This new feature will only work for web clients not on Android or Apple apps that doesn't not use web client? |
ah yeah, just for the web app. This functionality would need to be ported over to other repos like jellyfin-android and jellyfin-expo (iOS App). |
I think it should be a core jellyfin feature. It's a good idea to add ratings to the item cards. There would be a lot of interest for this in all clients as it's useful information. |
Both of those integrate the web client as their frontend, so no further changes would be needed there. FYI, @shoecar you have some unaddressed review feedback, in case you missed it. |
Changes
Two major changes:
cardRatings
display settingcardRatings
valueScreenshots


New "Display > Item Card" setting type with "Item Card Ratings Display" setting:
This controls which of
CriticRating
/CommunityRating
should be displayed (defaulting toNone
).Here is what it looks like with

All
selected:You can see no
CriticRating
is shown for shows as that data is not available. Moves have bothCriticRating
andCommunityRating
so they are shown together.Issues
3330/rating-on-posters
discussions/6911