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

fix(reactnative): fix accessibility for component preview (iOS+VoiceOver) #4601

Merged

Conversation

estevaolucas
Copy link

Issue: N/A

What I did

On React-Native, <StoryView> is wrapped by a <TouchableOpacity>, and by default touchable elements on RN has an accessible={true} (docs).

This causes this problem: VoiceOver won't focus on children elements when the parent has accessible={ true }, which means it's not possible to fully test the accessibility of components inside Storybook.

So the easiest solution for it is settting accessible={false} for the <TouchableOpacity>

How to test

Is this testable with Jest or Chromatic screenshots? N
Does this need a new example in the kitchen sink apps? N
Does this need an update to the documentation? N

If your answer is yes to any of these, please make sure to include it in your PR.

For maintainers only: Please tag your pull request with at least one of the following:
["cleanup", "BREAKING CHANGE", "feature request", "bug", "documentation", "maintenance", "dependencies", "other"]

@codecov
Copy link

codecov bot commented Oct 29, 2018

Codecov Report

Merging #4601 into master will decrease coverage by <.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4601      +/-   ##
==========================================
- Coverage   35.59%   35.58%   -0.01%     
==========================================
  Files         557      557              
  Lines        6732     6730       -2     
  Branches      884      883       -1     
==========================================
- Hits         2396     2395       -1     
  Misses       3876     3876              
+ Partials      460      459       -1
Impacted Files Coverage Δ
...-native/src/preview/components/OnDeviceUI/index.js 0% <ø> (ø) ⬆️
addons/knobs/src/components/types/Select.js 76.92% <0%> (-7.7%) ⬇️
lib/cli/lib/helpers.js 0.98% <0%> (+0.01%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 19352c7...ebb0dad. Read the comment docs.

@storybook-safe-bot
Copy link
Contributor

Fails
🚫

PR is not labeled with one of: ["cleanup","BREAKING CHANGE","feature request","bug","documentation","maintenance","dependencies:update","dependencies","other"]

Generated by 🚫 dangerJS

@estevaolucas
Copy link
Author

@Gongreg new contributor here (this is just my second PR, along with #4597). Do I need to fix this coverage decrease?

I hope I can contribute more with react-native related PRs. But I need to fix a workflow issue first.

I wasn't able to properly test my code changes. I run npm run build-packs to update packs folder, and inside the crna example I run npm install to get a fresh packaged, but it doesn't get updated. Looks like yarn workspaces has some cache that prevents the package to get updated.

How do you guys do to test your react-native changes?
Thanks.

@Gongreg
Copy link
Member

Gongreg commented Oct 29, 2018

Hey @elucaswork, glad to hear that you want to contribute with React Native.

Truth to be told whenever I want to install anything in react native I use:
rm -rf node_modules; rm package-lock.json; npm install

And you are correct, the flow is:

  • Update some code in monorepo.
  • Run npm run build-packs in root directory
  • cd to example project
  • rm -rf node_modules; rm package-lock.json; npm install

Can you update me if this helps?

Also if you want any ideas I wrote about them here: https://medium.com/storybookjs/whats-new-in-storybook-4-0-react-native-741c7f481bbb

@estevaolucas
Copy link
Author

@Gongreg awesome! I'm going to test this flow!

Do I need to fix this coverage decrease and make the CI pass? If so, do you have a suggestion on how to make it?

Also if you want any ideas I wrote about them here: https://medium.com/storybookjs/whats-new-in-storybook-4-0-react-native-741c7f481bbb

Nice work man! Thanks for doing it.

I want to start making some small improvements/fixes on it, but hopefully, I'll be able to make major contributions.

Example of improvement: <Navigation> in <OnDeviceUI> stays underneath the keyboard when testing some TextField components, making the user stuck (I should file an issue for it). Move <Navigation> to the top or use <KeyboardAvoidingView> might fix it.

@Gongreg
Copy link
Member

Gongreg commented Oct 30, 2018

@elucaswork Great catch with navigation. I suggest adding keyboard avoiding view. In the long run we will use webview for navigation, but it will still require keyboard avoiding view.

Regarding test coverage, don't mind it yet, we still don't test storybook properly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants