-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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 audible messages to confirm the "move block" actions #24894
Add audible messages to confirm the "move block" actions #24894
Conversation
To fully test, worth reminding the keyboard shortcuts to move blocks are (on macOS):
Moving left or right can be tested on the "Buttons" or "Social icons" blocks: Screenshot illustrating moving three social icons buttons when using Safari and VoiceOver: |
|
||
announceBlockMoved( state, direction ); | ||
}, | ||
MOVE_BLOCKS_UP: ( action, { getState } ) => { |
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.
It's odd to read an action type of "UP" while orientation changes it to right / left within. It'd be better to have a single "MOVE_BLOCKS" type and handle all cardinals within. (Alternatively, there could be four action types.)
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.
This just uses the existing actions. Actually, there are no specific actions for "left" and "right". Not opposed to improvements but the current actions implementation is out of the scope of this issue.
let direction = 'down'; | ||
|
||
if ( action.orientation === 'horizontal' ) { | ||
direction = settings.isRTL ? 'left' : 'right'; |
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.
Is this correct? Wouldn't the actual move actions still be accurate given left moves to the left regardless of RTL?
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.
It is correct. "down" moves the block to the "following" position, whether it's vertical or horizontal. In LTR, the "following" horizontal position is right, while in RTL is left.
const blockCount = getSelectedBlockCount( state ); | ||
let message; | ||
|
||
switch ( direction ) { |
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.
It seems all these could be inlined and don't need the extra abstraction of announceBlockMoved
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.
I'd disagree. Seems to me it's way more readable this way.
I tested again this PR, also on Windows Firefox + NVDA and in RTL. Everything works as expected. Code looks good as is, to me. Kindly asking to have a new review please, when there's a chance 🙂 /Cc @youknowriad @talldan In RTL: last action was "move to right": |
@@ -219,6 +271,30 @@ export default { | |||
) | |||
); | |||
}, | |||
MOVE_BLOCKS_DOWN: ( action, { getState } ) => { |
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.
Can we move away from effects and instead implement these action creator as "generators"? see moveBlocksToPosition
as an example of action generator.
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.
Not sure I follow. Maybe I lack context or it's a terminology thing 🙂 In this PR we just reused actions that already exist. Are you suggesting a major refactoring in the way these actions are created?
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.
We're slowly moving away from using "effects" (see effects.js file and the https://github.com/aduth/refx middleware) and instead we're trying to replace them with "generator" actions (consistency with resolvers).
These are just two technologies that allow the same thing: writing complex actions with side effects.
So ideally, new code shouldn't add new effects and use generators instead.
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.
Good to know. Is this documented anywhere? How new contributors are supposed to be informed about this? 🙂 If it's not documented, I'd say it's a barrier to new contributors...
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.
You're right, it should be. We were hoping we could get rid of these files which remove the confusion but the refactor efforts staled.
i created a new issue at #67237 to recap the current situation. |
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the Unlinked AccountsThe following contributors have not linked their GitHub and WordPress.org accounts: @jimmy@yoast.com. Contributors, please read how to link your accounts to ensure your work is properly credited in WordPress releases. If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.
To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
Thanks @afercia, I think it's better to start from scratch. I'll close this one. |
Fixes #23995.
Description
Added audible messages to
aria-live="assertive"
region to confirm the "move" action to the screen reader users.The messages check for orientation and RTL to make sure they are consistent with the label of the button used to perform the action.
This PR has been created during the Yoast Contributor Day.
The people who worked on it are @jcomack, @afercia, @enricobattocchi.
How has this been tested?
horizontal
orientation)<div id="a11y-speak-assertive">
to read the messages without a screen reader active.Types of changes
New feature (non-breaking change which adds functionality)
Checklist: