-
Notifications
You must be signed in to change notification settings - Fork 7.5k
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
feat: improved text tracks settings labels #8101
Conversation
💖 Thanks for opening this pull request! 💖 Things that will help get your PR across the finish line:
We get a lot of pull requests on this repo, so please be patient and we will get back to you as soon as we can. |
Codecov Report
@@ Coverage Diff @@
## main #8101 +/- ##
=======================================
Coverage 81.97% 81.97%
=======================================
Files 110 110
Lines 7339 7339
Branches 1770 1770
=======================================
Hits 6016 6016
Misses 1323 1323
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
Not that it shouldn't change, but the names are taken from the CVAA, which has these user settings requirements. |
Personally, I think the text changes are fine. But my concern with the layout is that the "Font Size", "Text Edge Style", and "Font Family" controls are so close to the three "Opacity" controls that it isn't clear visually that the "Opacity" fields are each part of a fieldset with the legend "Text", "Text Background", and "Caption Area Background". That could be fixed by increasing the spacing, or adding an outline for each fieldset. |
You're right, that does not look as expected, and nor did a lot of mobile devices when the menu was opened full screen. I updated this with bolded headers and a few small styling changes to make it look better in most cases. The only flaw I found is that in some medium sized players, the user is now forced to scroll to get to the bottom of the menu. If this is an issue let me know, but I believe in most cases this will look much better. EDIT: We set the size of the menu to 70 percent of the player. If that is increased to 90 or so, the entire menu would show in every case I believe, but at the cost of their being a bit more space between the actual menu and the buttons at the bottom of the screen in other cases where there is plenty of space. |
I think it's fine for now, even with the scrolling. I built it out about 10 years and 2 months ago, and it needs to be re-designed from scratch at this point. |
I agree with @mister-ben that a small indent to the left of the labeled settings (color, opacity) would also improve at-a-glance legibility. Overall, it looks much cleaner! |
@roman-bc-dev This was more work then I expected 😅 One of these label/select elements was wrapped in a span, the other was not.. so I updated the HTML to ensure that both are wrapped in a span element, updated the css, and also a few test fixes :) |
Congrats on merging your first pull request! 🎉🎉🎉 |
- remove IE-specific code, refer to videojs#7701 & videojs#7708 - remove remaining code to hide labels, refer to videojs#8101
Description
Based off of an audit that a Brightcove client received, we are updating the text tracks settings menu with improved labels. This change is regarding Web Content Accessibility Guidelines (WCAG).
Specific Changes proposed
Requirements Checklist