feat: i shortcut on workspace gives overview#9677
feat: i shortcut on workspace gives overview#9677mikeharv wants to merge 1 commit intoRaspberryPiFoundation:v13from
Conversation
BenHenning
left a comment
BenHenning
left a comment
There was a problem hiding this comment.
Thanks @mikeharv! Took a first pass and has some comments, PTAL.
Note that I'm not requesting changes in case you want to switch to a different reviewer since I'm actually out until next Wednesday.
|
|
||
| registerDefaultShortcuts(); | ||
| registerKeyboardNavigationShortcuts(); | ||
| registerScreenReaderShortcuts(); |
There was a problem hiding this comment.
I think we actually want to keep this out, for now. The idea is we'll use an optional plugin to enable these based on user prompting.
| Blockly.Msg.ARIA_WORKSPACE_COMMENTS_MANY = ' and %1 comments'; | ||
| /** @type {string} */ | ||
| /// ARIA live region phrase appended when there is exactly one workspace comment. | ||
| Blockly.Msg.ARIA_WORKSPACE_COMMENTS_ONE = ' and one comment'; No newline at end of file |
There was a problem hiding this comment.
Nit: please re-add EOF line terminator.
| const stacks = workspace.getTopBlocks().length; | ||
| const comments = workspace.getTopComments().length; |
There was a problem hiding this comment.
Nit: Perhaps use stackCount and commentCount for a bit more clarity on what's being contained in this variables (since they read a bit as arrays at the moment).
| "ARIA_WORKSPACE_BLOCKS_MANY": "%1 stacks of blocks%2 in workspace.", | ||
| "ARIA_WORKSPACE_BLOCKS_ONE": "One stack of blocks%2 in workspace.", | ||
| "ARIA_WORKSPACE_BLOCKS_ZERO": "No blocks%2 in workspace.", | ||
| "ARIA_WORKSPACE_COMMENTS_MANY": " and %1 comments", | ||
| "ARIA_WORKSPACE_COMMENTS_ONE": " and one comment" |
There was a problem hiding this comment.
I'm finding this a bit hard to grok, and I suspect a translator might as well. Perhaps we could be slightly repetitive to simplify things. What if we avoided the double concatenation and had four different variables for the label:
- "%1 stacks of blocks and %2 comments in workspace."
- "One stack of blocks and %2 comments in workspace."
- "%1 stacks of blocks and one comment in workspace."
- "One stack of blocks and one comment in workspace."
Separately, I'm not sure about the Msg name here. These aren't actual ARIA labels or properties, they're just strings that happen to be used as a status message to trick the screen reader into announcing them I do recognize that we will want to standardize on this, though, so perhaps @maribethb might have a suggestion. One thought might be to do something like: WORKSPACE_MANY_BLOCKS_AND_MANY_COMMENTS_SCREEN_READER_ANNOUNCEMENT_LABEL but I'm not sure sure just how specific we want to get with these.
| this.liveRegion = document.getElementById('blocklyAriaAnnounce'); | ||
| }); | ||
|
|
||
| test('Announces block stacks and comments', function () { |
There was a problem hiding this comment.
I suggest splitting these up a bit. The Blockly team doesn't exactly follow the arrange-act-assert structure strictly in the codebase but it's a good baseline to follow particularly for longer tests like this as more granular failures can be more informative when investigating.
2 participants
The basics
The details
Resolves
Fixes #9622
Proposed Changes
This adds a new 'i' shortcut for workspaces. If the workspace is focused, the shortcut will trigger an announcement of the current total of block stacks and workspace comments. New translation strings were added to facilitate communicating the number of stacks and comments in a grammatically accurate way. The new strings were intentionally phrased to front-load the most important information (ie. actual quantity) over contextual description (ie. "on the workspace").
See tests for sample announcement strings created using the new messages.
Reason for Changes
From linked issue:
Test Coverage
The main new test performs a round trip through the different permutations of messages. The other test checks that the precondition logic works as expected (the workspace must be focused).
Documentation
N/A
Additional Information
I expect we'll want to make sure this new shortcut gets added to the keyboard navigation help menu/shortcuts list once that has been ported over.