Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[Mobile]Update string concatenation for accessibility labels #15181

Merged
merged 4 commits into from
Apr 26, 2019
Merged

[Mobile]Update string concatenation for accessibility labels #15181

merged 4 commits into from
Apr 26, 2019

Conversation

pinarol
Copy link
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 etoledom and Tug 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
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
Contributor

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
Contributor Author

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
Contributor

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
Contributor Author

pinarol commented Apr 26, 2019

Ready for another look @etoledom

@pinarol
Copy link
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
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