-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Add easier links to video details from playing and playlist screens #7348
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?
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.
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
dmitrylyzo
left a comment
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.
Not tested, but to make it work on TV, title should have focusable class. Or it could be a link, then using href it can be opened in a new tab.
jellyfin-web/src/scripts/libraryMenu.js
Line 46 in c23b722
| html += '<h3 class="pageTitle" aria-hidden="true"></h3>'; |
| LibraryMenu.setTitle(title); | ||
|
|
||
| // Make the page title clickable to navigate to the item detail page | ||
| const pageTitleElement = document.querySelector('.pageTitle'); |
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.
Better to query from view rather than document (to narrow down the search).
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.
Oh, my bad, it belongs to the library menu.
As an option, we could add url argument to LibraryMenu.setTitle and add/remove click handling or add/change the link url there. Or maybe add another function for title "link", but setTitle should reset it anyway. I'm for setTitle(title, url = ''), but let's see what others say.
|
@dmitrylyzo thanks so much for the review! I made 3/4 changes. the title wasn't in the view, it's in the header, so not sure if there's a better way you'd want to refer to it but I didn't see one. I did add focusable on the h3 - if you'd rather do a link let me know. I have tested in browser but not on TV. |
You can change the mode in |
|
Thanks @dmitrylyzo ! I'm new to this codebase and your reviews so helpful. I have moved the title link logic to the library as you suggested, and I've made sure it works on TV mode (added a video in PR description). I set up the link to be focusable but didn't make it a button, so it looks the same as before in non-TV mode. It turns blue rather than highlighting when you focus it in TV mode. |
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.
|



Changes
I added two new links. While a video is playing, the video title was just text - I made it a link to video details. Super handy.
On a video playlist, I added a video details link in the context menu. Also super handy.
Issues
Fider page
Meta:
I've been wanting these two features for a while. I'm new to Jellyfin dev and I see there's a bit of a PR backlog here, sorry to add to it! Let me know how I can help.
Videos of changes:
Desktop, video playing title link:
https://github.com/user-attachments/assets/1ba9d224-4617-4db6-857c-ba603f63834d
Playlist context:
https://github.com/user-attachments/assets/aa08a8f4-940b-4206-a58a-e2ca58e041f1
TV, video playing focus state for title link:
tvmode.mov