feat: Add keyboard shortcut to perform an action on the currently focused element#9673
feat: Add keyboard shortcut to perform an action on the currently focused element#9673
Conversation
mikeharv
left a comment
mikeharv
left a comment
There was a problem hiding this comment.
This looks great. Thanks for your patience with my questions as some of this code is still somewhat new to me.
| * @param workspace The workspace. | ||
| */ | ||
| export function showHelpHint(workspace: WorkspaceSvg) { | ||
| const shortcut = getShortActionShortcut('list_shortcuts'); |
There was a problem hiding this comment.
Should we handle the potential case where shortcut is an empty string? I think it's possible that the shortcut could have been unregistered which would lead to a help prompt with a nonsensical message.
There was a problem hiding this comment.
That is a possibility, I'm not sure how we might helpfully handle it though. End users wouldn't be able to do anything about it, I suppose we could just not show the toast? Or did you have something else in mind?
There was a problem hiding this comment.
I think it's reasonable that if the shortcut isn't available because it was unregistered, we could just return early and not show the toast. No toast is better than one that says "Press for help on keyboard controls". The only other option I can think of would be a fallback string that just says "invalid action" or something if we don't have a shortcut to suggest for help.
There was a problem hiding this comment.
Added an early return in that case. I also disabled the test as it now fails since the shortcut to show the list of shortcuts doesn't yet exist.
| Blockly.Msg.KEYBOARD_NAV_CUT_HINT = 'Cut. Press %1 to paste.'; No newline at end of file | ||
| Blockly.Msg.KEYBOARD_NAV_CUT_HINT = 'Cut. Press %1 to paste.'; | ||
| /** @type {string} */ | ||
| /// Message shown when a user presses Enter with a navigable block focused. |
There was a problem hiding this comment.
Will translators need any more context? For example, is it still the right arrow key on LTR workspaces? (Genuine question I don't know the answer to.)
There was a problem hiding this comment.
I think we're OK, ideally this would interpolate the actually-bound shortcut like some of the others but that's something we can address later I think.
gonfunko
force-pushed
the
lights-camera-action
branch
from
c9dbb5b to
44a6acc
Compare
mikeharv
left a comment
mikeharv
left a comment
There was a problem hiding this comment.
Thanks for answering all of my questions. I think my biggest concern is the possibility of creating a toast with a broken/incomplete message if the shortcut has gone missing. This is hopefully more of an edge, but perhaps worth tracking with a new issue so we can remember to solve it at some point.
| * @param workspace The workspace. | ||
| */ | ||
| export function showHelpHint(workspace: WorkspaceSvg) { | ||
| const shortcut = getShortActionShortcut('list_shortcuts'); |
There was a problem hiding this comment.
I think it's reasonable that if the shortcut isn't available because it was unregistered, we could just return early and not show the toast. No toast is better than one that says "Press for help on keyboard controls". The only other option I can think of would be a fallback string that just says "invalid action" or something if we don't have a shortcut to suggest for help.
2 participants
The basics
The details
Resolves
Proposed Changes
This PR backports the Enter keyboard shortcut from the keyboard-experimentation repo that performs an action on the currently focused element. It adds an optional
performAction()method to IFocusableNode which may be adopted by classes to gain support for Enter acting upon them.