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

a11y accessibility plugin slow on larger components #3683

Closed
DerJacques opened this issue May 29, 2018 · 10 comments
Closed

a11y accessibility plugin slow on larger components #3683

DerJacques opened this issue May 29, 2018 · 10 comments

Comments

@DerJacques
Copy link

Bug or support request summary

The a11y accessibility plugin for good reasons takes some time to run its analysis.
We have a somewhat large component and it takes the plugin about 3 seconds to complete the analysis.

From reading the code, it appears that the accessibility analysis is run as soon as the component is mounted. Maybe it would make sense to run the analysis on request only?

Steps to reproduce

  1. Start a performance recording in chrome
  2. Mount a somewhat large component
  3. Search for "runA11yCheck"

I have attached a screenshot of the local test results.

Please specify which version of Storybook and optionally any affected addons that you're running

  • @storybook/react 3.4.3
  • @storybook/addon-a11y 3.4.3

Affected platforms

Chrome, Mac OS

Screenshots / Screencast / Code Snippets (Optional)

screen shot 2018-05-29 at 16 18 02

Solution suggestion

I think that it would make sense to run the analysis on demand only. So whenever the accessibility tab is actually opened. As of right now, the analysis is always run.

@ndelangen
Copy link
Member

@DerJacques I see that becoming a problem! I'd be happy to take a PR to change the behavior.

Do you think you'd be up for it? any help you'd need from me?

@DerJacques
Copy link
Author

DerJacques commented May 29, 2018

@ndelangen Thank you for the quick response! I just gave it a shot, but this would be my first contribution to Storybook and I have a hard time getting a development environment up and running that allows me to make changes to addon-a11y and have them compiled so I can see the changes of my code.

I used your Development guide to get me started, but still am unsure how I can make changes to not only the stories, but the addon I would like to work on. Can you point me in the right direction?

@Keraito
Copy link
Contributor

Keraito commented May 29, 2018

@DerJacques, I think this is what you're mainly looking for: https://github.com/storybooks/storybook/blob/master/CONTRIBUTING.md#initial-setup, specifically the 7. yarn dev You must have this running for your changes to show up part!

From my experience, the easiest is to do the initial setup, then run yarn dev at root (so your changes will be reflected in the examples), and then go to examples/official-storybook (or examples/cra-kitchen-sink) and then run yarn storybook (to launch the examples locally). Any changes you then apply and save in the addons-a11y of your project will be reflected in the example. Hope this can help you, feel free to ping me if you need help!

@ndelangen
Copy link
Member

@DerJacques I'd be more then happy to help you with that! Would you care to join our slack and we can possibly setup a online meeting where I can assist you.

invite to slack

@DerJacques
Copy link
Author

DerJacques commented May 30, 2018

@Keraito, thank you for the great explanation! My dev environment works perfectly now 👌

Back to the issue at hand:
My first intuition was to move the following line from /a11y/src/index.js#L26 to the addon panel's componentDidMount hook:

// storybook/addons/a11y/src/components/Panel.js
componentDidMount() {
  this.channel.on(Events.STORY_RENDERED, runA11yCheck);
  this.channel.on(CHECK_EVENT_ID, this.onUpdate);
}

However, it seems that the addon panels are actually all mounted and rendered all the time. They are just hidden using a display: none.

Is there any other way to check if an addon has been selected? Feels a bit bad to look at the URL for this, but that would be a possibility.

Changing the behaviour from display: none to a conditional rendering may be a different way to go, but this would potentially be a breaking change and may have unforeseen consequences.

@DerJacques
Copy link
Author

Thanks @ndelangen for the invitation! I am a bit busy at the moment but will try to get in touch asap. :)

@DerJacques
Copy link
Author

@ndelangen Fantastic! I guess it makes sense to wait for your branch to be merged in? :) If you remember to update this issue when the change is made that would be great. If not, I will make sure to check back in regularly.

@stale
Copy link

stale bot commented Jun 20, 2018

Hi everyone! Seems like there hasn't been much going on in this issue lately. If there are still questions, comments, or bugs, please feel free to continue the discussion. Unfortunately, we don't have time to get to every issue. We are always open to contributions so please send us a pull request if you would like to help. Inactive issues will be closed after 30 days. Thanks!

@stale stale bot added the inactive label Jun 20, 2018
@DerJacques
Copy link
Author

Fixed in #3690.

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

No branches or pull requests

3 participants