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

Add user setting for whether to display ratings in Video Player and Up Next Dialog Window #5774

Conversation

flebel9
Copy link

@flebel9 flebel9 commented Jul 11, 2024

Changes
Adds user setting under Playback section to control whether ratings should be displayed in the Video Player and Up Next Dialog Window.

Image of setting:
msedge_l3vXxguNka

Player bar with setting disabled and enabled:
msedge_I4pBRBPSsk
msedge_4s7VKNrZ4n

Next up window with setting disabled and enabled:

msedge_FqFuvfhY0C
msedge_2KZLxc9q9b

@flebel9 flebel9 requested a review from a team as a code owner July 11, 2024 21:15
src/components/upnextdialog/upnextdialog.js Show resolved Hide resolved
src/controllers/playback/video/index.js Show resolved Hide resolved
src/strings/en-gb.json Outdated Show resolved Hide resolved
@dmitrylyzo dmitrylyzo added enhancement Improve existing functionality or small fixes ui & ux This PR or issue mainly concerns UI & UX labels Jul 12, 2024
@flebel9 flebel9 force-pushed the add-setting-for-rating-playback-and-nextup branch from c0bfc89 to f2c8398 Compare July 12, 2024 09:28
return this.set('enableDisplayRatingsInPlayer', val.toString());
}

return toBoolean(this.get('enableDisplayRatingsInPlayer', false));
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't we enable it by default to keep the current behavior?

@@ -146,6 +146,14 @@ <h2 class="sectionTitle">
</div>
</div>

<div class="checkboxContainer checkboxContainer-withDescription">
Copy link
Contributor

Choose a reason for hiding this comment

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

I would place it before fldExternalPlayer. When the latter is displayed, the order is odd: NextVideoOverlay, ExternalPlayer, DisplayRatings (for next video overlay).

@nielsvanvelzen
Copy link
Member

My 2 cents on this; I'm not a fan of yet another setting for an extremely small detail. I do think those ratings are quite useless during playback so personally I'd remove them without a preference.

@flebel9
Copy link
Author

flebel9 commented Jul 28, 2024

My 2 cents on this; I'm not a fan of yet another setting for an extremely small detail. I do think those ratings are quite useless during playback so personally I'd remove them without a preference.

@dmitrylyzo Do you have a preferred approach for this? I am happy to continue the change to make this a preference or remove if that is the general opinion.

@dmitrylyzo
Copy link
Contributor

Ratings have been added in #4491 to close the feature request.

We don't visit item details to see rating for the next item, so it looks more useful in the NextUp dialog compared to the main OSD.

Since we don't have a clear opinion, I would make it customisable. But I'm not good at UI design. 😅

@jellyfin-bot
Copy link
Collaborator

Cloudflare Pages deployment

Latest commit 1ff7b02
Status 🔄 Deploying...
Preview URL Not available
Type 🔀 Preview

View bot logs

Copy link

sonarcloud bot commented Aug 4, 2024

@flebel9
Copy link
Author

flebel9 commented Aug 4, 2024

Ratings have been added in #4491 to close the feature request.

We don't visit item details to see rating for the next item, so it looks more useful in the NextUp dialog compared to the main OSD.

Since we don't have a clear opinion, I would make it customisable. But I'm not good at UI design. 😅

I thought about this and it does seem unnecessary to display ratings for the current video when they can be seen by going to the details page for the video in question which is more likely to happen for those who want it. The NextUp dialog looks to be where this should be rather than the main OSD in that case.
Would you be happy for me to remove this from the main OSD but keep it in the NextUp dialog?

@thornbill
Copy link
Member

I think my vote would be for no options. Remove it from the OSD and keep it in the next up dialog.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improve existing functionality or small fixes ui & ux This PR or issue mainly concerns UI & UX
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants