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.

Sign up for GitHub

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

Jump to bottom

Make Height editable on TV Channels #4181

Merged
merged 3 commits into from
Mar 7, 2024
Merged

Make Height editable on TV Channels #4181

merged 3 commits into from
Mar 7, 2024

Conversation

SenorSmartyPants
Copy link
Contributor

Changes

Editing metadata for a channel now allows user to set Height. Useful in determining which channels are HD.

Issues

Depends on jellyfin/jellyfin#8777
Useful in conjunction with jellyfin/jellyfin#8768

@sonarcloud
Copy link

sonarcloud bot commented Nov 20, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
1.1% 1.1% Duplication

@thornbill thornbill added the backend Requires work on the server to finish label Nov 20, 2022
@SenorSmartyPants
Copy link
Contributor Author

Back end has been merged.

@thornbill thornbill added enhancement Improve existing functionality or small fixes and removed backend Requires work on the server to finish labels Mar 23, 2023
@thornbill
Copy link
Member

I think from a ux standpoint it would be better to have a drop down menu for common resolutions.

@SenorSmartyPants
Copy link
Contributor Author

I think from a ux standpoint it would be better to have a drop down menu for common resolutions.

Dropdown added.

@thornbill thornbill added the needs testing This PR requires additional testing label Apr 12, 2023
@thornbill
Copy link
Member

I rebased this because sonarqube was flagging some issues that had been resolved since the PR was originally opened. The code changes look pretty good, I'm just adding a label so I remember to give it a quick test before approving + merging. 🙂

@sonarcloud
Copy link

sonarcloud bot commented Apr 12, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
1.0% 1.0% Duplication

@jellyfin-bot
Copy link
Collaborator

This pull request has merge conflicts. Please resolve the conflicts so the PR can be successfully reviewed and merged.

@jellyfin-bot jellyfin-bot added the merge conflict Conflicts prevent merging label Apr 12, 2023
@thornbill
Copy link
Member

@jellyfin-bot rebase

@jellyfin-bot
Copy link
Collaborator

I'm sorry @thornbill, I'm afraid I can't do that.

@SenorSmartyPants SenorSmartyPants requested a review from a team as a code owner September 24, 2023 21:11
@jellyfin-bot jellyfin-bot removed the merge conflict Conflicts prevent merging label Sep 24, 2023
@SenorSmartyPants
Copy link
Contributor Author

Rebase, fix merge conflict and force push.

@sonarcloud
Copy link

sonarcloud bot commented Sep 24, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
1.2% 1.2% Duplication

@thornbill
Copy link
Member

Setting the "height" of a TV channel seems like a really strange concept from a user standpoint. I think it would be better if we call it "resolution" and display standard resolutions as the options with values being the height.

I think I would just make the options SD, SD (PAL), HD, Full HD, UHD (4K).

@SenorSmartyPants
Copy link
Contributor Author

I used height as it is an existing field and kept the label. I have no problem with the concept of updating these labels.

                    <select is="emby-select" id="selectHeight" label="${LabelHeight}">
                        <option value="0"></option>
                        <option value="480">SD</option>
                        <option value="576">SD (PAL)</option>
                        <option value="720">HD</option>
                        <option value="1080">Full HD</option>
                        <option value="2160">UHD (4K)</option>
                    </select>

Or maybe those should be added as translatable resource strings.

Feel free to modify this PR as you wish.

@thornbill
Copy link
Member

Yeah to be clear I don't have any issues at all with the height field being used for the backend. I just think it can be presented in a more user friendly manner on the frontend. 🙂

@thornbill thornbill added this to the v10.9.0 milestone Jan 12, 2024
@thornbill thornbill removed the needs testing This PR requires additional testing label Mar 5, 2024
@sonarcloud SonarCloud
Copy link

sonarcloud bot commented Mar 5, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.9% Duplication on New Code

See analysis details on SonarCloud

@jellyfin-bot
Copy link
Collaborator

Cloudflare Pages deployment

Latest commit 8443b5d
Status ✅ Deployed!
Preview URL https://22217fb8.jellyfin-web.pages.dev
Type 🔀 Preview

View build logs
View bot logs

@thornbill thornbill merged commit 4487894 into jellyfin:master Mar 7, 2024
21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nielsvanvelzen nielsvanvelzen nielsvanvelzen approved these changes

Assignees
No one assigned
Labels
enhancement Improve existing functionality or small fixes
Projects
No open projects
Release 10.9.z
Status: Done
Milestone
v10.9.0
Development

Successfully merging this pull request may close these issues.
4 participants
@SenorSmartyPants @thornbill @jellyfin-bot @nielsvanvelzen