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

Addon-A11y: Show errors, reset config properly #8779

Merged
merged 3 commits into from Nov 14, 2019

Conversation

donaldpipowitch
Copy link
Contributor

@donaldpipowitch donaldpipowitch commented Nov 10, 2019

Issue: #8126

What I did

  1. Added proper error state.
  2. Reset configs properly and apply them partially.
  3. Removed // ... axe options comment in the README. It was ambiguous as it implied there were some "axe options" which could be spread into the configuration alongside element/ config/options.

image

How to test

This change fixes this demo: https://github.com/donaldpipowitch/storybook-bug-demo-a11y-at-story-level


Questions

  1. I needed to add // eslint-disable-next-line react/destructuring-assignment. If you don't like that change, I'd need to wrap A11YPanelState in another object, because descruturing doesn't work well with tagged union types in that case. Any preference?

  2. Is there a way to run $ yarn prepare in a --watch mode? That would help while debugging.

  3. Can I partially override a11y configs on a story level? The README implied it as far as I understood, but it isn't really written down literally. At least I always assumed it, but the code behaved like it would need the whole config even on a story level.

@vercel
Copy link

vercel bot commented Nov 10, 2019

This pull request is being automatically deployed with ZEIT Now (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://zeit.co/storybook/monorepo/o0ny2a1si
🌍 Preview: https://monorepo-git-fork-donaldpipowitch-fix-8126-a11y-addon-config.storybook.now.sh

@shilman shilman added addon: a11y bug patch:yes Bugfix & documentation PR that need to be picked to main branch help wanted labels Nov 10, 2019
@shilman
Copy link
Member

shilman commented Nov 11, 2019

@donaldpipowitch tests failing on something exactly relating to this PR:

  ● A11YPanel › should register STORY_RENDERED and RESULT updater on mount

    expect(received).toBe(expected) // Object.is equality

    Expected: 2
    Received: 3

      73 | 
      74 |     // then
    > 75 |     expect(api.on.mock.calls.length).toBe(2);
         |                                      ^
      76 |     expect(api.on.mock.calls[0][0]).toBe(STORY_RENDERED);
      77 |     expect(api.on.mock.calls[1][0]).toBe(EVENTS.RESULT);
      78 |   });

      at Object.it (addons/a11y/src/components/A11YPanel.test.js:75:38)

@donaldpipowitch
Copy link
Contributor Author

Thanks for the heads up. I just noticed https://github.com/storybookjs/storybook/runs/297077854.

@donaldpipowitch
Copy link
Contributor Author

Should be fixed now 👍

@shilman
Copy link
Member

shilman commented Nov 11, 2019

Cool, thanks. Gonna wait and see if @CodeByAlex or @jsomsanith-tlnd can review. But will review/merge if I don't hear back in a few days.

@shilman shilman changed the title fix 8126: show errors, reset config properly Addon-A11y: show errors, reset config properly Nov 14, 2019
@shilman shilman changed the title Addon-A11y: show errors, reset config properly Addon-A11y: Show errors, reset config properly Nov 14, 2019
@shilman shilman merged commit 2d18cba into storybookjs:next Nov 14, 2019
@donaldpipowitch donaldpipowitch deleted the fix-8126-a11y-addon-config branch November 14, 2019 11:47
@shilman shilman added the patch:done Patch/release PRs already cherry-picked to main/release branch label Nov 30, 2019
shilman added a commit that referenced this pull request Nov 30, 2019
Addon-A11y: Show errors, reset config properly
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
addon: a11y bug help wanted patch:done Patch/release PRs already cherry-picked to main/release branch patch:yes Bugfix & documentation PR that need to be picked to main branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants