Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add audible messages to confirm the "move block" actions #24894

Open
wants to merge 1 commit into
base: trunk
Choose a base branch
from
Open

Add audible messages to confirm the "move block" actions #24894

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion packages/block-editor/src/components/block-mover/button.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,7 @@ const BlockMoverButton = forwardRef(
direction === 'up' ? moveBlocksUp : moveBlocksDown;

const onClick = ( event ) => {
moverFunction( clientIds, rootClientId );
moverFunction( clientIds, rootClientId, orientation );
if ( props.onClick ) {
props.onClick( event );
}
Expand Down
46 changes: 27 additions & 19 deletions packages/block-editor/src/components/keyboard-shortcuts/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,23 +12,31 @@ import { __ } from '@wordpress/i18n';

function KeyboardShortcuts() {
// Shortcuts Logic
const { clientIds, rootBlocksClientIds, rootClientId } = useSelect(
( select ) => {
const {
getSelectedBlockClientIds,
getBlockOrder,
getBlockRootClientId,
} = select( 'core/block-editor' );
const selectedClientIds = getSelectedBlockClientIds();
const [ firstClientId ] = selectedClientIds;
return {
clientIds: selectedClientIds,
rootBlocksClientIds: getBlockOrder(),
rootClientId: getBlockRootClientId( firstClientId ),
};
},
[]
);
const {
clientIds,
rootBlocksClientIds,
rootClientId,
orientation,
} = useSelect( ( select ) => {
const {
getSelectedBlockClientIds,
getBlockOrder,
getBlockRootClientId,
getBlockListSettings,
} = select( 'core/block-editor' );
const selectedClientIds = getSelectedBlockClientIds();
const [ firstClientId ] = selectedClientIds;
const blockRootClientId = getBlockRootClientId( firstClientId );
const { orientation: blockListOrientation } =
getBlockListSettings( blockRootClientId ) || {};

return {
clientIds: selectedClientIds,
rootBlocksClientIds: getBlockOrder(),
rootClientId: blockRootClientId,
orientation: blockListOrientation || 'vertical',
};
}, [] );

const {
duplicateBlocks,
Expand All @@ -47,7 +55,7 @@ function KeyboardShortcuts() {
useCallback(
( event ) => {
event.preventDefault();
moveBlocksUp( clientIds, rootClientId );
moveBlocksUp( clientIds, rootClientId, orientation );
},
[ clientIds, moveBlocksUp ]
),
Expand All @@ -60,7 +68,7 @@ function KeyboardShortcuts() {
useCallback(
( event ) => {
event.preventDefault();
moveBlocksDown( clientIds, rootClientId );
moveBlocksDown( clientIds, rootClientId, orientation );
},
[ clientIds, moveBlocksDown ]
),
Expand Down
3 changes: 2 additions & 1 deletion packages/block-editor/src/store/actions.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -356,11 +356,12 @@ export function replaceBlock( clientId, block ) {
* @return {Function} Action creator.
*/
function createOnMove( type ) {
return ( clientIds, rootClientId ) => {
return ( clientIds, rootClientId, orientation ) => {
return {
clientIds: castArray( clientIds ),
type,
rootClientId,
orientation,
};
};
}
Expand Down
76 changes: 76 additions & 0 deletions packages/block-editor/src/store/effects.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ import {
getTemplate,
isValidTemplate,
getSelectionStart,
getSettings,
} from './selectors';

/**
Expand Down Expand Up @@ -66,6 +67,57 @@ export function validateBlocksToTemplate( action, store ) {
}
}

/**
* Announces when a block is moved.
*
* @param {Object} state The current state.
* @param {string} direction The direction the block was moved in.
*/
export function announceBlockMoved( state, direction ) {
const blockCount = getSelectedBlockCount( state );
let message;

switch ( direction ) {
Copy link
Member

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

Copy link
Contributor

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.

case 'up':
/* translators: %s: number of selected blocks */
message = _n(
'%s block moved up.',
'%s blocks moved up.',
blockCount
);
break;
case 'down':
/* translators: %s: number of selected blocks */
message = _n(
'%s block moved down.',
'%s blocks moved down.',
blockCount
);
break;
case 'left':
/* translators: %s: number of selected blocks */
message = _n(
'%s block moved left.',
'%s blocks moved left.',
blockCount
);
break;
case 'right':
/* translators: %s: number of selected blocks */
message = _n(
'%s block moved right.',
'%s blocks moved right.',
blockCount
);
break;
default:
/* translators: %s: number of selected blocks */
message = _n( '%s block moved.', '%s blocks moved.', blockCount );
}

speak( sprintf( message, blockCount, direction ), 'assertive' );
}

export default {
MERGE_BLOCKS( action, store ) {
const { dispatch } = store;
Expand Down Expand Up @@ -219,6 +271,30 @@ export default {
)
);
},
MOVE_BLOCKS_DOWN: ( action, { getState } ) => {
Copy link
Contributor

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.

Copy link
Contributor

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?

Copy link
Contributor

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.

Copy link
Contributor

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...

Copy link
Contributor

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.

const state = getState();
const settings = getSettings( state );

let direction = 'down';

if ( action.orientation === 'horizontal' ) {
direction = settings.isRTL ? 'left' : 'right';
Copy link
Member

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?

Copy link
Contributor

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.

}

announceBlockMoved( state, direction );
},
MOVE_BLOCKS_UP: ( action, { getState } ) => {
Copy link
Member

@mtias mtias Aug 29, 2020 •
edited
Loading

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.)

Copy link
Contributor

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.

const state = getState();
const settings = getSettings( state );

let direction = 'up';

if ( action.orientation === 'horizontal' ) {
direction = settings.isRTL ? 'right' : 'left';
}

announceBlockMoved( state, direction );
},
RESET_BLOCKS: [ validateBlocksToTemplate ],
MULTI_SELECT: ( action, { getState } ) => {
const blockCount = getSelectedBlockCount( getState() );
Expand Down