-
Notifications
You must be signed in to change notification settings - Fork 867
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 keyboard shortcuts to titles #5857
base: development
Are you sure you want to change the base?
Add keyboard shortcuts to titles #5857
Conversation
24b233d
to
0d3882e
Compare
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. |
ad59178
to
6202ff7
Compare
…has no matching value
6202ff7
to
7c379e2
Compare
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.
Pressing c
doesnt enable/disable captions
Added localization of shortcuts & multi-key shortcuts and caught a dangling instance of the wrong constant name for |
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. |
b934703
to
6f8d212
Compare
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.
Tested video player, sidebar, top nav button labels, refresh button
Only on MacOS though
// Add the keyboard shortcut to the label for the default Shaka controls | ||
|
||
const shakaControlKeysToShortcutLocalizations = new Map() | ||
Object.entries(shakaControlKeysToShortcuts).forEach(([shakaControlKey, shortcut]) => { | ||
const originalLocalization = localization.resolve(shakaControlKey) | ||
if (originalLocalization === '') { | ||
// e.g., A Shaka localization key in shakaControlKeysToShortcuts has fallen out of date and need to be updated | ||
console.error('Mising Shaka localization key "%s"', shakaControlKey) | ||
return | ||
} | ||
|
||
const localizationWithShortcut = addKeyboardShortcutToActionTitle( | ||
originalLocalization, | ||
shortcut | ||
) | ||
|
||
shakaControlKeysToShortcutLocalizations.set(shakaControlKey, localizationWithShortcut) | ||
}) | ||
|
||
localization.insert(shakaLocale, shakaControlKeysToShortcutLocalizations) |
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.
Non-blocking: For locales that shaka-player doesn't pre-bundle (so the ones loaded by the code above), we call .insert()
twice, once to add the strings and the second time to overwrite them with the shortcuts text. So in the future it might be worth considering having two separate code paths, the current one would be used for locales that are pre-bundled and a second one that combines the fetching and modifications in a single step so we only need to insert the strings once.
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. |
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.
Fullwinodow doesnt show shortcut key in overflow menu
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. |
correct |
…at/add-shortcut-labels
…ube into feat/add-shortcut-labels
Conflicts have been resolved. A maintainer will review the pull request shortly. |
8410df1
to
304fa82
Compare
84deb9e
to
cb7f395
Compare
cb7f395
to
532fcc7
Compare
Add keyboard shortcuts to titles
Pull Request Type
Related issue
closes #1587
Description
Mute (m)
Screenshots
Testing
Full screen (f)
/Exit full screen (f)
). See the official list of shortcuts for reference.Desktop
Additional context
The addition of the new constants should help make enhancements like #251 easier in the future.