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

Add addon-a11y to monorepo #2292

Merged
merged 30 commits into from
Nov 13, 2017
Merged

Add addon-a11y to monorepo #2292

merged 30 commits into from
Nov 13, 2017

Conversation

ndelangen
Copy link
Member

@ndelangen ndelangen commented Nov 11, 2017

Closes: storybook-eol/storybook-addon-a11y#14

What I did

Merged in the a11y addon into the monorepo

How to test

It's used in the cra-kitchen-sink

@ndelangen ndelangen self-assigned this Nov 11, 2017
@ndelangen ndelangen requested review from jbovenschen and a team November 11, 2017 21:10
Copy link
Member

@danielduan danielduan left a comment

Choose a reason for hiding this comment

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

Fix up those references to the old package name and move the package Storybook over to our CRA.

I think it's good to take a pass and see if this work with React 16 too. I don't see anything obvious that would break, but more testing is always good.

const text = Faker.lorem.words();

storiesOf('<Button />', module)
.addDecorator(checkA11y)
Copy link
Member

Choose a reason for hiding this comment

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

I think these a11y stories should be moved over to CRA so we can test them together.

Copy link
Member Author

Choose a reason for hiding this comment

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

agree

Copy link
Member Author

Choose a reason for hiding this comment

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

Added, will leave the old storybook, because might copy over some more stories later.

First, install the addon.

```sh
$ npm install -D storybook-addon-a11y
Copy link
Member

Choose a reason for hiding this comment

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

new package name?

import React from 'react';
import { storiesOf } from '@storybook/react';

import { checkA11y } from 'storybook-addon-a11y';
Copy link
Member

Choose a reason for hiding this comment

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

same here

"dependencies": {
"@storybook/react": "^3.2.14",
"axe-core": "^2.0.7",
"prop-types": "^15.5.10"
Copy link
Member

Choose a reason for hiding this comment

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

peer dep? also this shouldn't require @storybook/react

@@ -0,0 +1,3 @@
node_modules
coverage
dist
Copy link
Member

Choose a reason for hiding this comment

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

Looks like it's all covered by root .gitignore

@codecov
Copy link

codecov bot commented Nov 11, 2017

Codecov Report

Merging #2292 into master will decrease coverage by 0.94%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2292      +/-   ##
==========================================
- Coverage   22.14%   21.19%   -0.95%     
==========================================
  Files         268      283      +15     
  Lines        5893     6156     +263     
  Branches      710      724      +14     
==========================================
  Hits         1305     1305              
- Misses       4060     4302     +242     
- Partials      528      549      +21
Impacted Files Coverage Δ
addons/a11y/src/components/Report/Elements.js 0% <0%> (ø)
addons/a11y/register.js 0% <0%> (ø)
addons/a11y/src/components/Report/Item.js 0% <0%> (ø)
addons/a11y/src/components/Tabs.js 0% <0%> (ø)
addons/a11y/src/components/Report/index.js 0% <0%> (ø)
addons/a11y/src/components/Report/Info.js 0% <0%> (ø)
addons/a11y/src/register.js 0% <0%> (ø)
addons/a11y/src/A11yManager.js 0% <0%> (ø)
addons/a11y/manager.js 0% <0%> (ø)
addons/a11y/src/components/WrapStory.js 0% <0%> (ø)
... and 46 more

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 7a1f7c4...0f30fc9. Read the comment docs.

@ndelangen
Copy link
Member Author

Added a simple example to cra-kitchen-sink:

screen shot 2017-11-12 at 00 41 22

@ndelangen
Copy link
Member Author

Seems to work fine with React 16.1.x!

@@ -0,0 +1,3 @@
// NOTE: loading addons using this file is deprecated!
Copy link
Member

Choose a reason for hiding this comment

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

Why?

@danielduan
Copy link
Member

Code looks good to go, you'll need to rebase and the tests though.

@ndelangen
Copy link
Member Author

Should be OK to merge now, @Hypnosphi doe you agree?


storiesOf('Addon a11y', module)
.addDecorator(checkA11y)
.add('Default', () => <BaseButton />)
Copy link
Member

Choose a reason for hiding this comment

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

This produces a propType warning:

Warning: Failed prop type: The prop `label` is marked as required in `BaseButton`, but its value is `undefined`

Copy link
Member Author

Choose a reason for hiding this comment

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

Will fix!

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.

7 participants