Skip to content

[Mobile]Update string concatenation for accessibility labels#15181

Merged
pinarol merged 4 commits intomasterfrom
rnmobile/update-string-concatenation-for-accessibility
Apr 26, 2019
Merged

[Mobile]Update string concatenation for accessibility labels#15181
pinarol merged 4 commits intomasterfrom
rnmobile/update-string-concatenation-for-accessibility

Conversation

@pinarol
Copy link
Copy Markdown
Contributor

@pinarol pinarol commented Apr 25, 2019

Description

Improves some accessibility labels and adds comments to make the localization process easier, based on the feed back at #15161

How has this been tested?

Tested via gutenberg mobile PR.

Types of changes

Bug fix (non-breaking change which fixes an issue)

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.

@pinarol pinarol added the Mobile App - i.e. Android or iOS Native mobile impl of the block editor. (Note: used in scripts, ping mobile folks to change) label Apr 25, 2019
@pinarol pinarol self-assigned this Apr 25, 2019
@pinarol pinarol requested review from Tug and etoledom April 25, 2019 15:39
@pinarol pinarol mentioned this pull request Apr 25, 2019
Merged
8 tasks
@pinarol pinarol force-pushed the rnmobile/update-string-concatenation-for-accessibility branch from bb9a492 to cadb99b Compare April 25, 2019 15:47
etoledom
etoledom reviewed Apr 26, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@etoledom etoledom left a comment

Choose a reason for hiding this comment

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

Working good!

I found a small detail on Image Block, described on code comments. The rest looks shinny ✨

Another small detail not related to labels:

The Image block is marked as a 'button' to show that it is actionable. Other blocks are not marked as a button but it says 'actions available'.

In general, I personally prefer mark everything that is actionable with double tap, that doesn't have a more specific trait (like link), as button. If what the button action does is not obvious, there we can use hints.

packages/block-library/src/image/edit.native.js Outdated
__( 'Image caption. Empty.' ) :
sprintf(
/* translators: accessibility text. %s: image caption. */
__( 'Image caption. Empty. %s' ),
Copy link
Copy Markdown
Contributor

@etoledom etoledom Apr 26, 2019

Choose a reason for hiding this comment

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

Here I believe it shouldn't say Empty.

Copy link
Copy Markdown
Contributor Author

@pinarol pinarol Apr 26, 2019

Choose a reason for hiding this comment

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

copy paste error :(

packages/block-library/src/image/edit.native.js Outdated
accessibilityLabel={
isEmpty( caption ) ?
/* translators: accessibility text. Empty image caption. */
__( 'Image caption. Empty.' ) :
Copy link
Copy Markdown
Contributor

@etoledom etoledom Apr 26, 2019

Choose a reason for hiding this comment

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

There's a dot at the end that seems to not be needed/expected.

@pinarol pinarol requested a review from etoledom April 26, 2019 11:30
@pinarol
Copy link
Copy Markdown
Contributor Author

pinarol commented Apr 26, 2019

Ready for another look @etoledom

@pinarol
Copy link
Copy Markdown
Contributor Author

pinarol commented Apr 26, 2019 •
edited
Loading

The Image block is marked as a 'button' to show that it is actionable. Other blocks are not marked as a button but it says 'actions available'.

I did some experiments about when the system says 'actions available', when not. It looks like it is related with child components of the block. When there are some actionable components like UITextField inside the block, the system decides saying 'actions available'. For Image block for example, if there's a caption available then it says 'actions available' but if there's not then it is not saying anything(since we aren't rendering the text field). So it looks like we can still benefit from having the button role to indicate available actions.

etoledom
etoledom approved these changes Apr 26, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@etoledom etoledom left a comment

Choose a reason for hiding this comment

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

Thank you for tiding this up, and for the investigation!

So it looks like we can still benefit from having the button role to indicate available actions.

I agree on keeping this as it is for now 👍

@pinarol pinarol merged commit f083177 into master Apr 26, 2019
@pinarol pinarol deleted the rnmobile/update-string-concatenation-for-accessibility branch April 26, 2019 13:38
This was referenced Apr 29, 2019
Merged
Merged
hypest pushed a commit that referenced this pull request May 2, 2019
@pinarol @hypest
[Mobile]Update string concatenation for accessibility labels (#15181)
5a09c65 
* Make accessibility labels properly localizable

* Fix indentation

* Fix image block accessibility labels
@youknowriad youknowriad added this to the 5.6 (Gutenberg) milestone May 13, 2019
This was referenced Sep 12, 2024
Open
Open
Open
@AKJUS AKJUS mentioned this pull request Sep 19, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@etoledom etoledom etoledom approved these changes

@Tug Tug Awaiting requested review from Tug

Assignees

@pinarol pinarol

Labels

Mobile App - i.e. Android or iOS Native mobile impl of the block editor. (Note: used in scripts, ping mobile folks to change)

Projects

None yet

Milestone

5.6 (Gutenberg)

Development

Successfully merging this pull request may close these issues.

3 participants

@pinarol @etoledom @youknowriad