Update/block mover accessibility

[njpanderson]

Most of the changes I've made have been discussed in #557 but feel free to let me know if you need more information. Very aware this is my first contribution so it might not be up to scratch!

[dmsnell]

Have you considered using an early-abort style instead of doing more nesting and mutation of title?

if ( isFirst && isLast ) {
	return `${ errorPrefix } is the only block and cannot be moved`;
}

// moving down and last block
if ( dir > 0  && isLast ) {
	return `${ errorPrefix } is at the beginning of the content and can't be moved up`;
}

// moving down and not first block
if ( dir > 0 && ! isLast ) {
	return `${ prefix } down to position ${ position + 1 }`;
}

// moving up and first block
if ( dir < 0 && isFirst ) {
	return `${ errorPrefix } … `
}

// moving up and not first block
if ( dir < 0 && ! isFirst ) {
	return `${ prefix }…`;
}

// no other valid options, return empty
return '';

[dmsnell]

looks like some good stuff here!

@njpanderson I'm not as familiar with translation in Gutenberg but I'm wondering if we couldn't restructure some of the strings to make translation easier. That would mean taking those string variable prefixes and inlining them into the value returned.

I left a note about code style and early-return/early-abort vs. nesting and mutation. I personally find that much easier to track because things aren't changing as we scan down the line, plus it gives us a nice way to line up similar conditions in a similar shape.

hope this helps! (just random thoughts, please don't hold up your work because I wrote something here)

[njpanderson]

Thank you for the comments, @dmsnell !

1. Regarding early aborts - absolutely fine with that, and can make the changes if necessary?
2. Translation - is there a good way to get these strings set up for translation now? Happy to do work to make them compatible.

For the prefixes, I suspect the reason I used them is obvious, given the repetition, but If the translation side of things means that repetition is necessary then I'm happy to inline them all too.

As for the technicalities of PRs, should I submit another one once I've made the changes on my branch, or will this one update somehow when I've pushed more commits?

[njpanderson]

Have made my changes above and they appear to have magically appeared here. Still no translation, as I'm not entirely sure how it should be handled with regards to concatenating dynamic values.

[youknowriad]

I think we can't make this change because the block mover is also used with several blocks (multi-selection). See the second usage in this exact same file

[njpanderson]

Ah, thank you for spotting and apologies for not seeing it myself! I'll look into amending this tonight and allowing for the case of multiple blocks. Likely the string will just have to read something like "Move n blocks {down|up} by one place".

[aduth]

To your question on translation, the i18n utility includes an sprintf implementation for string format replacement. To translate, this could be written as:

/**
 * WordPress dependencies
 */
import { __, sprintf } from 'i18n';

// ...

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

[aduth]

The subtle difference here in returning an empty string vs. returning nothing at all is that if it reaches this point and an empty string is returned, it will cause an empty aria-label to be assigned to the button. We may want to remove this line altogether (an implicit undefined return), or return null; if we want to be explicit.

[njpanderson]

Hi all - I'm going to drop this request for now. Currently going around in circles with the logic and have hit a few road blocks. I'm confident of being able to sort this, but right now I think taking what I've learned so far and starting fresh from a more recent tip would be beneficial. Will PR the contrast change alone tonight as it's tiny, then give you the improvements to the label in the next request.