-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Changes from all commits
032d581
8b85043
f8e5f20
bb865f6
1b47eca
c5697b9
45ef29d
351134d
2465af5
73edbd4
4a87eb9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,109 @@ | ||
/** | ||
* Wordpress dependencies | ||
*/ | ||
import { __, sprintf } from 'i18n'; | ||
|
||
/** | ||
* 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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looking at the output of 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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, | ||
} | ||
); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,133 @@ | ||
/** | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice work on the tests 👍 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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' | ||
); | ||
} ); | ||
} ); | ||
} ); |
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.
Needs to have the dependencies header comments.