Add contextually aware title for block mover control (#557) #984

njpanderson · 2017-06-01T18:43:22Z

After thinking about what would be best for the user in terms of accessibility and guidance, I’ve decided that trying to give them the “content” of a block wouldn’t very useful in a lot of contexts – especially non-text blocks.

Instead, I’ve opted for telling them where the block is, and where it’ll be moved to.

This is also the case for multiple selected blocks, where it will fall back to simpler language.

After thinking about what would be best for the user in terms of accessibility and guidance, I’ve decided that trying to give them the “content” of a block wouldn’t very useful in a lot of contexts – especially non-text blocks. Instead, I’ve opted for telling them where the block is, and where it’ll be moved to. This is also the case for multiple selected blocks, where it will fall back to simpler language.

jasmussen · 2017-06-02T08:09:28Z

Thanks for working on this. I see no visual change, so I'm assuming it's for screenreaders. Which is ⭐️ .

mtias · 2017-06-02T09:27:07Z

editor/block-mover/index.js


-function BlockMover( { onMoveUp, onMoveDown, isFirst, isLast } ) {
+function BlockMover( { onMoveUp, onMoveDown, isFirst, isLast, uids, block, firstPosition } ) {


Why are both firstPosition and isFirst needed?

mtias · 2017-06-02T09:29:30Z

editor/block-mover/index.js

+	let blockType;
+
+	if ( uids.length === 1 && block ) {
+		blockType = getBlockType( block.name );


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.

mtias · 2017-06-02T09:31:04Z

editor/block-mover/mover-label.js

@@ -0,0 +1,73 @@
+import { __, sprintf } from 'i18n';


Needs to have the dependencies header comments.

mtias · 2017-06-02T09:33:05Z

editor/block-mover/mover-label.js

@@ -0,0 +1,73 @@
+import { __, sprintf } from 'i18n';
+
+export function blockMoverLabel( selectedCount, { type, firstPosition, isFirst, isLast, dir } ) {


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

njpanderson · 2017-06-02T17:57:29Z

@mtias - All above requests should have been handled in the most recent commits.

To answer the more specific queries:

Why are both firstPosition and isFirst needed?

firstPosition is an index integer whereas isFirst is an existing boolean defining whether the block is first. I've renamed firstPosition to make clearer as to its use.

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.

This is done, although I haven't extended to the .title property of the resulting instance of blockTitle as the syntax looked a bit unwieldy in the context of the connect arguments. Happy to change if you think it's worth doing though?

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

Absolutely, and in my haste to get this over I'd completely forgotten about docs. Have added full JSDoc tags - although I'm not sure where you guys put new typedefs, so I've left them at the top of the mover-label.js file. Also renamed the functions with a get prefix.

njpanderson · 2017-06-02T18:01:07Z

@jasmussen It is indeed! I've tested this with Voiceover and it works well. The change is intended to alter the aria-label property of the mover buttons and to fix #557.

njpanderson · 2017-06-03T13:19:23Z

Just reminding people that I don't have merge abilities here - This code looks to be in a high traffic area of the repo so I'd be grateful if someone could re-review after the recent updates. Not sure how you manage this so apologies if I'm stepping on toes!

@aduth

As suggested by @aduth in a previous PR, I’ve removed this (possibly unreachable) return and if none of the above cases are met, undefined will return which React can use to send an non-value prop, avoiding an empty aria-label attribute.

aduth · 2017-06-05T17:37:38Z

editor/block-mover/mover-label.js

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


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.

aduth · 2017-06-05T17:44:34Z

editor/block-mover/mover-label.js

+ * @param  {blockMoverLabelOptions} options       Object options.
+ * @return {string}                               Label for the block movement controls.
+ */
+export function getBlockMoverLabel( selectedCount, { type, firstIndex, isFirst, isLast, dir } ) {


I'm not sure I understand the distinction between selectedCount as a standalone argument vs. blockMoverLabelOptions object. Why not have these all as named arguments ( selectedCount, type, firstIndex, isFirst, isLast, dir ) or all as object arguments ( { selectedCount, type, firstIndex, isLast, dir } )?

aduth · 2017-06-05T17:45:16Z

editor/block-mover/test/mover-label.js

@@ -0,0 +1,121 @@
+/**


Nice work on the tests 👍

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

@aduth

As suggested by @aduth, I’ve converted the second argument in both functions to separate named arguments. I’m not as keen on this signature but it makes for much tidier documentation (as well as removing the need for custom type definitions).

njpanderson · 2017-06-05T20:32:42Z

That's those updates made, @aduth. Let me know what you think.

aduth

With variable rename, I think this should be good to go 👍

aduth · 2017-06-06T19:56:08Z

editor/block-mover/mover-label.js

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


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.

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.

aduth · 2017-06-06T19:58:11Z

editor/block-mover/test/mover-label.js

+import { getBlockMoverLabel, getMultiBlockMoverLabel } from '../mover-label';
+
+describe( 'block mover', () => {
+	const dir_up = -1,


Variables should be named as camelCase, per guidelines:

https://make.wordpress.org/core/handbook/best-practices/coding-standards/javascript/#naming-conventions

Not sure if we really need the variables at all really, or at least not exclusively in the context of tests, since it doesn't really do anything to safeguard us against refactoring of the code under test.

Ugh, sorry about that @aduth - In my defence this is my first ever WP contribution! Will read up on the standards and get better acquainted with the way things are done here.

On another note, I see eslint does have camelCase enforcement built in and wonder if we'd benefit from putting it into the .eslintrc file? Happy to do another PR if you'd like to add this.

Just to clarify - the variables dirUp and dirDown exist purely to guide future testers as to what the last argument is for. One could argue "why not just have all the arguments named", but the others felt too varied to be worth being named. Am happy to just remove the vars though, if you feel it would be more consistent with the rest of the test suites.

My worry is: Why is this relevant to the tests only, and not runtime usage?

The alternative I see is exporting constants from mover-label.js:

export const DIRECTION_UP = -1; export const DIRECTION_DOWN = 1;

(Realizing now that JavaScript standards have no equivalent to PHP's upper-case constants)

These constants could be imported both in tests, but also in <BlockMover />, and can be revised once and apply everywhere.

But still, I think it might be overkill. I'm not really requesting any changes here, just trying to understand why we'd care to special-case the tests.

My worry is: Why is this relevant to the tests only, and not runtime usage?

For runtime usage, it's only invoked from two statements in block-mover/index.js - the only reason I added these variables was to avoid the repetition of literals specifically within the tests. You're right though, when other arguments are still static it feels a bit odd.

I'm not massively wedded to it at all and am more than happy just to submit another PR removing them.

You might be onto something with the direction constants though — I could, however, make them static properties of the getBlockMoverLabel function:

function getBlockMoverLabel() { ... } getBlockMoverLabel.DIRECTION_UP = -1; getBlockMoverLabel.DIRECTION_DOWN = 1; export { getBlockMoverLabel };

Happy to do something like that if you'd like, assuming there's no issue with the constant syntax suggested above.

njpanderson changed the title ~~Add contextually aware title block mover (#557)~~ Add contextually aware title for block mover control (#557) Jun 1, 2017

jasmussen requested a review from youknowriad June 2, 2017 08:09

mtias reviewed Jun 2, 2017

View reviewed changes

editor/block-mover/mover-label.js

@@ -0,0 +1,73 @@

import { __, sprintf } from 'i18n';

Copy link

Member

mtias Jun 2, 2017

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.

mtias reviewed Jun 2, 2017

View reviewed changes

mtias added [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Feature] Blocks Overall functionality of blocks labels Jun 2, 2017

njpanderson added 6 commits June 2, 2017 18:42

Rename firstPosition to firstIndex

8b85043

Move blockTitle logic into connect

f8e5f20

Move getBlockType import into correct chunk

bb865f6

Add documentation for blockMoverLabel, etc

1b47eca

Update multi/blockMoverLabel with ‘get’ prefix

c5697b9

Fix test objects to use updated property

45ef29d

Remove empty return value to avoid empty prop.

351134d

As suggested by @aduth in a previous PR, I’ve removed this (possibly unreachable) return and if none of the above cases are met, undefined will return which React can use to send an non-value prop, avoiding an empty aria-label attribute.

aduth requested changes Jun 5, 2017

View reviewed changes

njpanderson added 2 commits June 5, 2017 19:17

Improve translation placeholders, add notes

73edbd4

aduth requested changes Jun 6, 2017

View reviewed changes

Update case of variables

4a87eb9

aduth approved these changes Jun 7, 2017

View reviewed changes

aduth merged commit 378f441 into WordPress:master Jun 7, 2017

aduth mentioned this pull request Jun 7, 2017

i18n: Traverse through parent paths to find translator comment #1068

Merged

njpanderson deleted the update/block-mover-labels branch June 8, 2017 06:27

aduth mentioned this pull request Jun 8, 2017

Blue circle hover effects on top of arrows #1051

Closed

nylen mentioned this pull request Jun 9, 2017

Improve the block mover buttons accessibility #557

Closed

afercia mentioned this pull request Jul 2, 2017

Add an aria-label to the editor blocks. #1618

Merged

dimadin mentioned this pull request Aug 9, 2018

i18n: Use argument swapping instead of named arguments for string placeholders #8771

Merged

snyk-bot mentioned this pull request Feb 1, 2020

[Snyk] Fix for 1 vulnerabilities SeppPenner/gutenberg#114

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add contextually aware title for block mover control (#557) #984

Add contextually aware title for block mover control (#557) #984

njpanderson commented Jun 1, 2017

jasmussen commented Jun 2, 2017

mtias Jun 2, 2017

mtias Jun 2, 2017

mtias Jun 2, 2017

mtias Jun 2, 2017

njpanderson commented Jun 2, 2017

njpanderson commented Jun 2, 2017 •

edited

Loading

njpanderson commented Jun 3, 2017

aduth Jun 5, 2017

aduth Jun 5, 2017

aduth Jun 5, 2017

njpanderson Jun 5, 2017

njpanderson commented Jun 5, 2017

aduth left a comment

aduth Jun 6, 2017

njpanderson Jun 6, 2017

aduth Jun 6, 2017

njpanderson Jun 6, 2017

njpanderson Jun 6, 2017

aduth Jun 7, 2017

njpanderson Jun 7, 2017


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

		@@ -0,0 +1,73 @@
		import { __, sprintf } from 'i18n';

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

Add contextually aware title for block mover control (#557) #984

Add contextually aware title for block mover control (#557) #984

Conversation

njpanderson commented Jun 1, 2017

jasmussen commented Jun 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

njpanderson commented Jun 2, 2017

njpanderson commented Jun 2, 2017 • edited Loading

njpanderson commented Jun 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

njpanderson commented Jun 5, 2017

aduth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

njpanderson commented Jun 2, 2017 •

edited

Loading