Add keyboard shortcuts to titles
Add keyboard shortcuts to titles
Pull Request Type
- [ ] Bugfix
- [X] Feature Implementation
- [ ] Documentation
- [ ] Other
Related issue
closes #1587
Description
- Adds keyboard shortcut information to their button labels, e.g.,
Mute (m)- This includes the video player buttons, the feed refresh buttons, the side nav History/Settings titles, and the top nav HistoryForward/HistoryBackward/OpenNewWindow buttons, but NOT Electron context menu labels
- Implementation note: To do this properly for the built-in Shaka controls that we don't modify in our code, this PR updates the base Shaka label localization values to include the shortcut whenever the Shaka locale is set.
- Implementation note: Constants were added for the aforementioned keyboard shortcuts.
Screenshots
Testing
- Test that buttons with corresponding keyboard shortcuts have the shortcut referenced in the label, be they in "on" or "off" mode (e.g.,
Full screen (f)/Exit full screen (f)). See the official list of shortcuts for reference.
Desktop
- OS: OpenSUSE
- OS Version: TW
Additional context
The addition of the new constants should help make enhancements like #251 easier in the future.
Label for captions
Another good catch; I wasn't testing with a video that had a captions option. Just fixed the issue and added logic preventing label modification if the Shaka localization key does not exist. Also note that this specific Captions label will be the only one with another parenthetical present. This is fine IMO.
Added localization of shortcuts & multi-key shortcuts and caught a dangling instance of the wrong constant name for CAPTIONS
Question: Navigating to the History tab and Settings tab also got shortcuts. Is there a reason those weren't added?
@efb4f5ff-1298-471a-8973-3d47447115dc I specifically added the shortcuts with corresponding button labels. The closest equivalent to those would be the corresponding side nav options, which I suppose would work. I can add those as well.
Still on the fence about whether we should use the Mac key symbols for darwin (⌘/⇧/⌥/etc.)
Edit: did it. I used the version of the arrow icons that are used in Mac context menus, but it is worth noting that FF for Mac uses the more standard-looking ←→↑↓. I'm also open to using the arrow icons for non-Mac systems; I just went with the text version of those from looking at FF for Linux.
Still on the fence about whether we should use the Mac key symbols for
darwin(⌘/⇧/⌥/etc.)Edit: did it. I used the version of the arrow icons that are used in Mac context menus, but it is worth noting that FF for Mac uses the more standard-looking ←→↑↓. I'm also open to using the arrow icons for non-Mac systems; I just went with the text version of those from looking at FF for Linux.
@PikachuEXE what do you think of the change in the latest commit
I think there is no need to use short form on labels The places I see short forms are app menu bar & keymap menus with limited spaces Even short forms should be used the arrows chars should be ←→↑↓ at least I haven't checked how this looks on Windows yet
Update 1: Checked windows version
- MacOS only symbol: prefer text, probably fine with symbol too if someone else want it
- Arrow: Just use text
Zen Browser
RubyMine
Vivaldi
I can change the arrow symbols. I'm not sure if we have other active contributors who are Mac users to provide additional opinions, but maybe if any users have any thoughts
Updated the arrow icons for Mac:
I preferred the stats button before this pull request a lot more as it matched the appearance of the other controls, now it looks out of place in the player.
Before:
After:
Other controls:
Ah, for the stats button, you're referring to the text content? I noticed that the labels we were setting for it weren't being used, so I thought that was an error. I can change that back if desired.
I can also look into removing the shortcuts from the overflow menu if that is desired as well. My only concern with that is that I'm not sure if we will be adding more shortcuts or moving more controls to a default overflow menu in the future.
Sorry, I'm still a bit confused on what is being asked to be changed. Just fixing full window to have the shortcut show in the overflow menu? @absidue @efb4f5ff-1298-471a-8973-3d47447115dc
This pull request has conflicts, please resolve those before we can evaluate the pull request.
Just fixing full window to have the shortcut show in the overflow menu?
correct
Conflicts have been resolved. A maintainer will review the pull request shortly.