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.

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

Update accessibility related APIs #1326

Merged
merged 9 commits into from
Nov 6, 2019
Merged

Conversation

xuelgong
Copy link
Contributor

@xuelgong xuelgong commented Oct 4, 2019

This PR adds the new accessibility state in facebook/react-native#24608 and accessibility value in facebook/recat-native#26169. Also, clean up the accessibility APIs in View, Text and TouchableWithoutFeedback.

@react-native-bot
Copy link

react-native-bot commented Oct 4, 2019

Deploy preview for react-native ready!

Built with commit 2fdffbc

https://deploy-preview-1326--react-native.netlify.com

Changes to docs/ are reflected in the next "master" version.

Thank you for your contributions.

How to ContributeDocumentation Sources

@cpojer cpojer requested a review from rachelnabors October 11, 2019 07:08
@cpojer
Copy link
Contributor

cpojer commented Oct 11, 2019

@rachelnabors could you review this one? :)

Copy link
Contributor

@rachelnabors rachelnabors 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 so much for these extensive updates to the accessibility docs! This is such an important contribution!

There is one point of clarity I must ask for your help on: the use of "should" is confusing in some places. For instance:

accessibilityValue is an object. It should contain the following fields:

Yet the table indicates none of the fields are required. Does that mean that the object doesn't need to contain any of them? If so, perhaps it would be clearer to say:

accessibilityValue is an object. It contains the following fields:

Then in the table that follows, all of the rows say "required: no," but in the text that follows, it's clear that they are required if now is set:

  • min The minimum value of this component's range. Should be an integer. Required if now is set.

Can the table be made to reflect that by saying "required if now is set?

Also the use of "should" in properties:

Should be an integer.

Should is ambiguous. It's an integer.

Integer.

I wonder if all this information would be clearer bundled into a single table rather than a table and a list?

Name Description Type Required
min The minimum value of this component's range. integer Required if now is set.
max The maximum value of this component's range. integer Required if now is set.
now The current value of this component's range. integer No
text A textual description of this component's value. Will override min, now, and max if set. integer No

@charpeni charpeni added the Wait on future Release This indicates a PR that updates the doc to match a future release. label Oct 11, 2019
@xuelgong
Copy link
Contributor Author

@rachelnabors Thanks for the comments. I'll update the doc as you suggested.

@xuelgong
Copy link
Contributor Author

@rachelnabors Could you help me merge this PR? Thanks!

@xuelgong xuelgong requested a review from rachelnabors October 31, 2019 01:25
@rachelnabors
Copy link
Contributor

Wow! So it seems my Github notifications have been going to a blackhole account for sometime. Thank you so much for your patience!

I made some minor edits and structural changes.

I am wondering, is there a reason these properties didn't get code examples?

  • accessibilityIgnoresInvertColors(iOS)
  • accessibilityRole (not sure we could do examples for ALL of them, but one would be nice!)
  • accessibilityState (iOS, Android)
  • accessibilityValue
  • accessibilityViewIsModal
  • accessibilityElementsHidden
  • onAccessibilityTap
  • onMagicTap
  • onAccessibilityEscape

I feel like in the upcoming documentation restructuring, this might get split into several pages for readability—a properties reference, a testing guide, and most importantly: a how-to guide.

Speaking of how-to guide, right now it's a bit difficult to see how all the pieces fit together. Is it possible to start the whole page with a bit of an explainer of why and how to go about making a component accessible? I feel like you do that with Accessibility Actions, but it's all the way below Accessibility Properties and doesn't cover the basics (like setting accessible to true). Is a wee sample tutorial at the top within scope for you? We can accept it the way it is now, but this would be such a help to future readers who want to make their apps accessible!

Thank you again so much for all your hard work on this!

@xuelgong
Copy link
Contributor Author

xuelgong commented Nov 6, 2019

@rachelnabors I am not the original author of this page so I don't know why not all the props have code examples. The purpose of this PR is to document the newly-added accessibility props - accessibilityState and accessibilityValue. So bigger refactoring on the doc is out of the scope of this PR. Could we merge this PR as it is now first? We have the plan to write up a guidance about how to make the component accessible and already start the work internally. We'll definitely contribute it back to the community when the doc is ready.

@rachelnabors
Copy link
Contributor

Great, so glad someone is thinking ahead on the documentation! Please feel free to put us in touch! And thank you for contributing these docs!

@rachelnabors rachelnabors merged commit 3b962a6 into facebook:master Nov 6, 2019
espipj pushed a commit to espipj/react-native-website that referenced this pull request Feb 8, 2020
* Update accessibility related APIs

* Add accessibilityValue prop to Touchable

* Modify the accessibilityValue prop description

* Using 'clear' instead of obvious

* Update touchablewithoutfeedback.md

Using 'clear' instead of obvious

* Moving more content into tables and reducing h-tag hierarchy

* Retooling opening paragraph and meta-description

* Tweaking intro copy
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Wait on future Release This indicates a PR that updates the doc to match a future release.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants