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.

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 contextually aware title for block mover control (#557) #984

Merged
merged 11 commits into from
Jun 7, 2017
27 changes: 25 additions & 2 deletions editor/block-mover/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,26 +13,47 @@ import { IconButton } from 'components';
* Internal dependencies
*/
import './style.scss';
import { isFirstBlock, isLastBlock } from '../selectors';
import { isFirstBlock, isLastBlock, getBlockOrder, getBlock } from '../selectors';
import { blockMoverLabel } from './mover-label';
import { getBlockType } from 'blocks';

function BlockMover( { onMoveUp, onMoveDown, isFirst, isLast } ) {
function BlockMover( { onMoveUp, onMoveDown, isFirst, isLast, uids, block, firstPosition } ) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are both firstPosition and isFirst needed?

// We emulate a disabled state because forcefully applying the `disabled`
// attribute on the button while it has focus causes the screen to change
// to an unfocused state (body as active element) without firing blur on,
// the rendering parent, leaving it unable to react to focus out.
let blockType;

if ( uids.length === 1 && block ) {
blockType = getBlockType( block.name );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the only thing you are using blockType for is the title, it'd make sense to move this to the connect function and assign a blockTitle prop there.

}

return (
<div className="editor-block-mover">
<IconButton
className="editor-block-mover__control"
onClick={ isFirst ? null : onMoveUp }
icon="arrow-up-alt2"
label={ blockMoverLabel( uids.length, {
type: blockType && blockType.title,
isFirst,
isLast,
firstPosition,
dir: -1,
} ) }
aria-disabled={ isFirst }
/>
<IconButton
className="editor-block-mover__control"
onClick={ isLast ? null : onMoveDown }
icon="arrow-down-alt2"
label={ blockMoverLabel( uids.length, {
type: blockType && blockType.title,
isFirst,
isLast,
firstPosition,
dir: 1,
} ) }
aria-disabled={ isLast }
/>
</div>
Expand All @@ -43,6 +64,8 @@ export default connect(
( state, ownProps ) => ( {
isFirst: isFirstBlock( state, first( ownProps.uids ) ),
isLast: isLastBlock( state, last( ownProps.uids ) ),
firstPosition: getBlockOrder( state, first( ownProps.uids ) ),
block: getBlock( state, first( ownProps.uids ) ),
} ),
( dispatch, ownProps ) => ( {
onMoveDown() {
Expand Down
73 changes: 73 additions & 0 deletions editor/block-mover/mover-label.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
import { __, sprintf } from 'i18n';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs to have the dependencies header comments.


export function blockMoverLabel( selectedCount, { type, firstPosition, isFirst, isLast, dir } ) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It'd be good to add a function comment here describing purpose and spec. Also, maybe getBlockMoverLabel is a better name.

const position = ( firstPosition + 1 );

if ( selectedCount > 1 ) {
return multiBlockMoverLabel( selectedCount, { isFirst, isLast, firstPosition, dir } );
}

if ( isFirst && isLast ) {
return sprintf( __( 'Block "%s" is the only block, and cannot be moved' ), type );
}

if ( dir > 0 && ! isLast ) {
// moving down
return sprintf(
__( 'Move "%s" block from position %s down to position %s' ),
type,
position,
( position + 1 )
);
}

if ( dir > 0 && isLast ) {
// moving down, and is the last item
return sprintf( __( 'Block "%s" is at the end of the content and can’t be moved down' ), type );
}

if ( dir < 0 && ! isFirst ) {
// moving up
return sprintf(
__( 'Move "%s" block from position %s up to position %s' ),
type,
position,
( position - 1 )
);
}

if ( dir < 0 && isFirst ) {
// moving up, and is the first item
return sprintf( __( 'Block "%s" is at the beginning of the content and can’t be moved up' ), type );
}

return '';
}

export function multiBlockMoverLabel( selectedCount, { isFirst, isLast, firstPosition, dir } ) {
const position = ( firstPosition + 1 );

if ( dir < 0 && isFirst ) {
return __( 'Blocks cannot be moved up as they are already at the top' );
}

if ( dir > 0 && isLast ) {
return __( 'Blocks cannot be moved down as they are already at the bottom' );
}

if ( dir < 0 && ! isFirst ) {
return sprintf(
__( 'Move %s blocks from position %s up by one place' ),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To accommodate languages which might swap placeholder ordering, it's recommended to use either argument swapping or named placeholders.

If you have more than one placeholder in a string, it is recommended that you use argument swapping.

https://codex.wordpress.org/I18n_for_WordPress_Developers#Placeholders

There's a few other instances of this in the file.

Examples:

__( 'Move %1$s blocks from position %2$s up by one place' ),
__( 'Move %(selectedCount)s blocks from position %(position)s up by one place' ),

Additionally, a %d format type (numeric) might be the more applicable type to use here.

selectedCount,
position
);
}

if ( dir > 0 && ! isLast ) {
return sprintf(
__( 'Move %s blocks from position %s down by one place' ),
selectedCount,
position
);
}
}
121 changes: 121 additions & 0 deletions editor/block-mover/test/mover-label.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,121 @@
/**
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work on the tests 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. Hopefully that's all the coverage you'd need.

* External dependencies
*/
import { expect } from 'chai';

/**
* Internal dependencies
*/
import { blockMoverLabel, multiBlockMoverLabel } from '../mover-label';

describe( 'block mover', () => {
describe( 'blockMoverLabel', () => {
const type = 'TestType';

it( 'Should generate a title for the first item moving up', () => {
expect( blockMoverLabel( 1, {
type,
firstPosition: 0,
isFirst: true,
isLast: false,
dir: -1,
} ) ).to.equal(
`Block "${ type }" is at the beginning of the content and can’t be moved up`
);
} );

it( 'Should generate a title for the last item moving down', () => {
expect( blockMoverLabel( 1, {
type,
firstPosition: 3,
isFirst: false,
isLast: true,
dir: 1,
} ) ).to.equal(
`Block "${ type }" is at the end of the content and can’t be moved down`
);
} );

it( 'Should generate a title for the second item moving up', () => {
expect( blockMoverLabel( 1, {
type,
firstPosition: 1,
isFirst: false,
isLast: false,
dir: -1,
} ) ).to.equal(
`Move "${ type }" block from position 2 up to position 1`
);
} );

it( 'Should generate a title for the second item moving down', () => {
expect( blockMoverLabel( 1, {
type,
firstPosition: 1,
isFirst: false,
isLast: false,
dir: 1,
} ) ).to.equal(
`Move "${ type }" block from position 2 down to position 3`
);
} );

it( 'Should generate a title for the only item in the list', () => {
expect( blockMoverLabel( 1, {
type,
firstPosition: 0,
isFirst: true,
isLast: true,
dir: 1,
} ) ).to.equal(
`Block "${ type }" is the only block, and cannot be moved`
);
} );
} );

describe( 'multiBlockMoverLabel', () => {
it( 'Should generate a title moving multiple blocks up', () => {
expect( multiBlockMoverLabel( 4, {
firstPosition: 1,
isFirst: false,
isLast: true,
dir: -1,
} ) ).to.equal(
'Move 4 blocks from position 2 up by one place'
);
} );

it( 'Should generate a title moving multiple blocks down', () => {
expect( multiBlockMoverLabel( 4, {
firstPosition: 0,
isFirst: true,
isLast: false,
dir: 1,
} ) ).to.equal(
'Move 4 blocks from position 1 down by one place'
);
} );

it( 'Should generate a title for a selection of blocks at the top', () => {
expect( multiBlockMoverLabel( 4, {
firstPosition: 1,
isFirst: true,
isLast: true,
dir: -1,
} ) ).to.equal(
'Blocks cannot be moved up as they are already at the top'
);
} );

it( 'Should generate a title for a selection of blocks at the bottom', () => {
expect( multiBlockMoverLabel( 4, {
firstPosition: 2,
isFirst: false,
isLast: true,
dir: 1,
} ) ).to.equal(
'Blocks cannot be moved down as they are already at the bottom'
);
} );
} );
} );