Add paragraph block writing flow tests#821
Conversation
…github.com/wordpress-mobile/gutenberg-mobile into add/paragraph-flow-tests
…github.com/wordpress-mobile/gutenberg-mobile into add/paragraph-flow-tests
…editor page and include test multiple paragraph blocks
Generated by 🚫 dangerJS |
| const { order } = this.props; | ||
| let moveUpButtonTitle = `Move up from row ${ order }`; | ||
| if ( this.props.canMoveUp ) { | ||
| moveUpButtonTitle += ` to row ${ order - 1 }`; |
There was a problem hiding this comment.
So, we have learned some bits about best practices for accessibility labels and localization.
In my opinion, this construction for labels looks good, but it's not localized.
Check this for an example how are we localizing accessibility labels:
https://github.com/WordPress/gutenberg/blob/f257ed207fa7cb1dc28e4b1f277af000240fe992/packages/block-library/src/image/edit.native.js#L259-L264
Note the use of sprintf for formatted strings, the /* translators: ... comment (that will be readable by the translator in GlotPress), and that the whole sentence is written together to give the translator the best context.
There was a problem hiding this comment.
Yeah thanks @etoledom totally going to do this, I'm having trouble resolving issues with the merged in changes from develop, taking longer than I expected so that's why I haven't pushed that update yet... as soon as I push it I'll ping for the review...
There was a problem hiding this comment.
Hey @JavonDavis !
I think is better to not append strings, since for the translators, the string __( ' to row %d' ) will be lacking some context. Beside there are no comments to help them.
I was thinking on something more like this: https://gist.github.com/etoledom/7162a547141490f76c885c0ecd7792ed
This proposed one in the gist looks a bit messy, but we ensure that the whole string gets to the translators and we add a comment that they can read via /* translators: ....
Of course I'm open to suggestions and to improve this proposal too :)
There was a problem hiding this comment.
Ohh I see, I didn't consider that there'd be more context needed around this... Thanks! Your proposed change I think covers it well
etoledom
left a comment
There was a problem hiding this comment.
Thank you for the update @JavonDavis !
I added a couple of comments about accessibility labels, but it's looking quite good already.
Please let me know what do you think about the requested changes.
|
|
||
| const removeButtonTitle = sprintf( | ||
| /* translators: accessibility text. %1: current block position (number). */ | ||
| __( 'Remove row %1$s' ), |
There was a problem hiding this comment.
I think this would be better if it says Remove block at row %s, since the action is to remove a block.
Note that when there's just one variable, %1$s is not necessary, just %s.
There was a problem hiding this comment.
I think we had previously discussed taking the word block out of the text since the block name would have already been said in VoiceOver once the block is selected, WDYT?
| order, | ||
| order - 1 ) : sprintf( | ||
| /* translators: accessibility text. %d: block position (number) */ | ||
| __( 'Move up from row %1$s' ), |
There was a problem hiding this comment.
It would be great if we can say here just Move up, for this case where the block can't be moved up.
The system will add a dimmed | disabled to let the user know that the action is disabled. And the user will knowledge the intent of the button.
There was a problem hiding this comment.
I think both messages are ok but, I agree removing the row number in this case this lines up better states the intent of the button when disabled and can be better for accessibility. As it relates to the tests this shouldn't have any impact so 👍
| order, | ||
| order + 1 ) : sprintf( | ||
| /* translators: accessibility text. %d: block position (number) */ | ||
| __( 'Move down from row %1$s' ), |
There was a problem hiding this comment.
If we change Move up, it would be nice to change this one too. 👍
Co-Authored-By: etoledom <etoledom@icloud.com>
…rg-mobile into add/paragraph-flow-tests
etoledom
left a comment
There was a problem hiding this comment.
Looks good! Thank you @JavonDavis for all your hard work 🎉
…ss-mobile/gutenberg-mobile into tests/add-list-test
Add list block test
…ge-not-loaded Re-load the ReactVideoPackage that got removed by #821
Part of #745
This PR adds the following tests covering writing flows with the Paragraph block
Additional notes
Tests on Android failing due to "Add new block" fails after first use #783BlockInteractions and uses functions from the editor-page class to interact with blocks on the editor screen