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
25 changes: 22 additions & 3 deletions editor/block-mover/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,31 +8,48 @@ import { first, last } from 'lodash';
* WordPress dependencies
*/
import { IconButton } from 'components';
import { getBlockType } from 'blocks';

/**
* Internal dependencies
*/
import './style.scss';
import { isFirstBlock, isLastBlock } from '../selectors';
import { isFirstBlock, isLastBlock, getBlockOrder, getBlock } from '../selectors';
import { getBlockMoverLabel } from './mover-label';

function BlockMover( { onMoveUp, onMoveDown, isFirst, isLast } ) {
function BlockMover( { onMoveUp, onMoveDown, isFirst, isLast, uids, blockType, firstIndex } ) {
// 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.

return (
<div className="editor-block-mover">
<IconButton
className="editor-block-mover__control"
onClick={ isFirst ? null : onMoveUp }
icon="arrow-up-alt2"
label={ getBlockMoverLabel(
uids.length,
blockType && blockType.title,
firstIndex,
isFirst,
isLast,
-1,
) }
aria-disabled={ isFirst }
/>
<IconButton
className="editor-block-mover__control"
onClick={ isLast ? null : onMoveDown }
icon="arrow-down-alt2"
label={ getBlockMoverLabel(
uids.length,
blockType && blockType.title,
firstIndex,
isFirst,
isLast,
1,
) }
aria-disabled={ isLast }
/>
</div>
Expand All @@ -43,6 +60,8 @@ export default connect(
( state, ownProps ) => ( {
isFirst: isFirstBlock( state, first( ownProps.uids ) ),
isLast: isLastBlock( state, last( ownProps.uids ) ),
firstIndex: getBlockOrder( state, first( ownProps.uids ) ),
blockType: getBlockType( getBlock( state, first( ownProps.uids ) ).name ),
} ),
( dispatch, ownProps ) => ( {
onMoveDown() {
Expand Down
109 changes: 109 additions & 0 deletions editor/block-mover/mover-label.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,109 @@
/**
* Wordpress dependencies
*/
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.


/**
* Return a label for the block movement controls depending on block position.
*
* @param {number} selectedCount Number of blocks selected.
* @param {string} type Block type - in the case of a single block, should
* define its 'type'. I.e. 'Text', 'Heading', 'Image' etc.
* @param {number} firstIndex The index (position - 1) of the first block selected.
* @param {boolean} isFirst This is the first block.
* @param {boolean} isLast This is the last block.
* @param {number} dir Direction of movement (> 0 is considered to be going
* down, < 0 is up).
* @return {string} Label for the block movement controls.
*/
export function getBlockMoverLabel( selectedCount, type, firstIndex, isFirst, isLast, dir ) {
const position = ( firstIndex + 1 );

if ( selectedCount > 1 ) {
return getMultiBlockMoverLabel( selectedCount, firstIndex, isFirst, isLast, dir );
}

if ( isFirst && isLast ) {
// translators: %s: Type of block (i.e. Text, Image etc)
Copy link
Member

Choose a reason for hiding this comment

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

Looking at the output of languages/gutenberg.pot after running npm run gettext-strings, it appears these comments aren't being picked up. Running the code through ASTExplorer, it seems to be an issue that the comment isn't considered a leadingComments for the actual translation call. It can be fixed by the following:

return sprintf( 
  /* translators: %s: Type of block (i.e. Text, Image etc) */
  __( 'Block "%s" is at the end of the content and can’t be moved down' ),
  type
);

However, I don't necessarily think you ought to need to make this change. I'd prefer if we could instead improve the tooling to be a bit more liberal in what it considers to be a leading comment for a translation, even if the actual function call occurs within sprintf or is the return value.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Have left as-is for now, but if it is decided that the comment should need updated, it's a tiny change and I'm happy to make it.

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

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

if ( dir > 0 && isLast ) {
// moving down, and is the last item
// translators: %s: Type of block (i.e. Text, Image etc)
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 "%(type)s" block from position %(position)d up to position %(newPosition)d' ),
{
type,
position,
newPosition: ( position - 1 ),
}
);
}

if ( dir < 0 && isFirst ) {
// moving up, and is the first item
// translators: %s: Type of block (i.e. Text, Image etc)
return sprintf( __( 'Block "%s" is at the beginning of the content and can’t be moved up' ), type );
}
}

/**
* Return a label for the block movement controls depending on block position.
*
* @param {number} selectedCount Number of blocks selected.
* @param {number} firstIndex The index (position - 1) of the first block selected.
* @param {boolean} isFirst This is the first block.
* @param {boolean} isLast This is the last block.
* @param {number} dir Direction of movement (> 0 is considered to be going
* down, < 0 is up).
* @return {string} Label for the block movement controls.
*/
export function getMultiBlockMoverLabel( selectedCount, firstIndex, isFirst, isLast, dir ) {
const position = ( firstIndex + 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 %(selectedCount)d blocks from position %(position)d up by one place' ),
{
selectedCount,
position,
}
);
}

if ( dir > 0 && ! isLast ) {
return sprintf(
__( 'Move %(selectedCount)d blocks from position %(position)s down by one place' ),
{
selectedCount,
position,
}
);
}
}
133 changes: 133 additions & 0 deletions editor/block-mover/test/mover-label.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,133 @@
/**
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 { getBlockMoverLabel, getMultiBlockMoverLabel } from '../mover-label';

describe( 'block mover', () => {
const dirUp = -1,
dirDown = 1;

describe( 'getBlockMoverLabel', () => {
const type = 'TestType';

it( 'Should generate a title for the first item moving up', () => {
expect( getBlockMoverLabel(
1,
type,
0,
true,
false,
dirUp,
) ).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( getBlockMoverLabel(
1,
type,
3,
false,
true,
dirDown,
) ).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( getBlockMoverLabel(
1,
type,
1,
false,
false,
dirUp,
) ).to.equal(
`Move "${ type }" block from position 2 up to position 1`
);
} );

it( 'Should generate a title for the second item moving down', () => {
expect( getBlockMoverLabel(
1,
type,
1,
false,
false,
dirDown,
) ).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( getBlockMoverLabel(
1,
type,
0,
true,
true,
dirDown,
) ).to.equal(
`Block "${ type }" is the only block, and cannot be moved`
);
} );
} );

describe( 'getMultiBlockMoverLabel', () => {
it( 'Should generate a title moving multiple blocks up', () => {
expect( getMultiBlockMoverLabel(
4,
1,
false,
true,
dirUp,
) ).to.equal(
'Move 4 blocks from position 2 up by one place'
);
} );

it( 'Should generate a title moving multiple blocks down', () => {
expect( getMultiBlockMoverLabel(
4,
0,
true,
false,
dirDown,
) ).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( getMultiBlockMoverLabel(
4,
1,
true,
true,
dirUp,
) ).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( getMultiBlockMoverLabel(
4,
2,
false,
true,
dirDown,
) ).to.equal(
'Blocks cannot be moved down as they are already at the bottom'
);
} );
} );
} );