Addon-a11y: Support manual run #8883

donaldpipowitch · 2019-11-19T09:34:52Z

What I did

refactored A11YPanel a lot:
- properly type icons (no any)
- properly type state with multiple possible status
- we now have an initial stat, so we don't show an empty result which isn't related to any story on startup
- I removed the ActionBar for the running state (personal opinion that this was confusing UX, because it was still clickable to trigger a run even though it was already running - I can revert that change, if you want to keep it)
a11y checks can now run manually 🥳

It would be nice, if the ESLint rules default-case,consistent-return,no-case-declarations could be removed completely for .ts/.tsx files. They should not be relevant anymore as TypeScript should check, if everything is declared correctly/every switch branch is covered/return value is correct. For now I ignored these checks in this file 😔

How to test

This is testable with Jest, however I probably need some guidance here. I had some troubles executing the tests locally. 😕 If I find time I try it again later this day or in the next days. But any help how to fix something properly would be useful.

I probably need to add api.emit(EVENTS.MANUAL, true); and api.emit(EVENTS.MANUAL, false); at the right places.

vercel · 2019-11-19T09:34:57Z

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/42b4n7zh3
✅ Preview: https://monorepo-git-fork-donaldpipowitch-8126-addon-a11y-922c94.storybook.now.sh

CodeByAlex · 2019-11-19T09:41:23Z

addons/a11y/src/components/A11YPanel.tsx

+}
+
+interface ReadyState {
+  status: 'ready';
  passes: Result[];


Couldn’t these 4 be made into a single interface with options for status? In other words, extending the old interface.

It could be merged, but I made this change on purpose. I wanted to use a state machine like approach (without introducing a dedicated lib like xstate) to make the code more easy to reason about.

The old interface looked simple, because it was just one interface, but it was (imho) harder to understand, because it was not very explicit. There was no differentiation between the initial state and the ready state for example, so there was the need to mock a proper result. But inside the component you never knew, if the result was mocked or not. (Also the user briefly saw "invalid" results, because of this.)

I could have added mocked results to the initial, manual and running states. They wouldn't "hurt", but they also wouldn't have any purpose. But it get's harder if you introduce new states which need other data (like error).

tl;dr: Yes, the interface became more verbose. But it also became more correct and explicit and I hope this will make it easier to understand and extend the logic of this component.

Long term, this addon should start using the useAddonState tbh.

@ndelangen Sorry, just saw your comment now. Did I understand you correctly that the approach is fine for now and useAddonState should be used in a future PR? Or should I add useAddonState to this PR?

donaldpipowitch · 2019-11-25T20:17:34Z

I updated the tests. I hope they are easier to read and extend now. 👋

donaldpipowitch · 2019-12-02T09:03:28Z

@shilman could you have a look? I just tried to resolve the latest conflicts.

ndelangen · 2019-12-06T12:21:43Z

@donaldpipowitch I've added the 5.4 milestone to this, We're currently in a feature freeze so we can release a stable 5.3.

donaldpipowitch · 2019-12-06T16:39:29Z

Alright. Thank you for the update :)

ndelangen · 2019-12-21T21:21:51Z

@donaldpipowitch If you think this is good to merge into the 6.0.0 branch, you can press the merge button 😄

donaldpipowitch · 2019-12-22T10:08:59Z

Thank you @ndelangen for the update 😊 Sadly I can't press on merge as there is a an approval needed by someone with write access. Have nice holidays everyone. ❤️

gaetanmaisse · 2019-12-31T08:53:23Z

addons/a11y/src/components/A11YPanel.tsx

+        status: 'manual',
+      });
+    } else {
+      this.request();


Are we sure that the story has been rendered before calling this.request()? 🤷‍♂ I'm just asking because STORY_RENDERED event was used before

It seemed to work for me without problems, but I don't know the internals. Maybe @ndelangen could confirm if events emitted inside makeDecorator -> wrapper (see https://github.com/storybookjs/storybook/pull/8883/files/d73733d99e2250d840f35e16044e8de69bdc9b6c#diff-135e5fd1db00e7a987038977b6cb9148R70) happen after rendering? If not, do you know an example I could add to the tests to see when it would fail, so I can fix it?

STORY_RENDERED is called after the awaited render function resolved/returned:
https://github.com/storybookjs/storybook/blob/next/lib/core/src/client/preview/start.js#L240-L241

So I do believe it's called later. Whether or not this impacts the a11y execution, I don't know.

There actually seems to be a problem - not sure if it's because of the missing STORY_RENDERED or some other change I made. If another panel is initially active and you switch to the a11y panel you only see the "Initializing..." state.

E.g. if you visit https://monorepo-51csjv7sk.now.sh/cra-kitchen-sink/?path=/story/app--full-app for example (taken from a different pull request) and switch to the a11y you see results. While you only see the "Initializing..." state (at least on first try - if you reload it works) if you run my pull request locally.

I probably need to fix that 😭

Good catch @donaldpipowitch, yes we need to fix this, I will try to take a look tonight if I have the time ;)

ndelangen · 2020-01-11T00:18:20Z

@gaetanmaisse could you resolve the merge conflicts here, and merge if all seems well?

gaetanmaisse · 2020-01-11T12:55:33Z

@ndelangen 👌I self-assigned me this issue.
After resolving conflicts there will still be an issue to fix: #8883 (comment) I will work on it too 😉

I will do it as soon as I can.

donaldpipowitch · 2020-01-12T10:34:23Z

Thank you for also working on that bug and getting everything to the finish line 👏

gaetanmaisse · 2020-01-12T19:50:10Z

addons/a11y/src/components/A11YPanel.tsx

@@ -20,26 +20,29 @@ export enum RuleType {
  INCOMPLETION,
 }

-const RotatingIcons = styled(Icons)(({ theme }) => ({
+const Icon = styled(Icons)({


@donaldpipowitch I needed to keep an Icon component with height and width otherwise the check icon from e368e57#diff-3250ce08c136d40dd47659131333a6a7R213 is not displayed

gaetanmaisse · 2020-01-12T20:15:23Z

addons/a11y/src/components/A11YPanel.tsx

+      this.request();
+    }
+  };
+
  request = () => {


@donaldpipowitch I succeeded to fix locally the issue we talk about a few days ago (https://github.com/storybookjs/storybook/pull/8883/files?file-filters%5B%5D=.snap&file-filters%5B%5D=.ts&file-filters%5B%5D=.tsx#r364712456) by removing the if-check in the request function. Can you try on your side? and add a commit in your branch if it works?

request = () => { const { api } = this.props; this.setState( { status: 'running', }, () => { api.emit(EVENTS.REQUEST); // removes all elements from the redux map in store from the previous panel store.dispatch(clearElements()); } ); };

I don't well understand why this active check is done, I played with SB examples locally and didn't find any new issue 🤷‍♂

@gaetanmaisse thank you for having a look. This seems to fix the issue - it now works for me locally.

I'm unsure about active as well. I mostly left this untouched. But there is an old comment which says that it might can be removed completely?

storybook/addons/a11y/src/components/A11YPanel.tsx

Line 100 in 2da3a4e

// TODO: might be able to remove this

For now I just added your fix and kept other usages of active.

I tried to simply remove all active usages and it breaks everything so I think we need to keep it for now.

…pipowitch/storybook into 8126-addon-a11y--allow-manual-run

gaetanmaisse · 2020-01-13T16:13:56Z

As this PR is about adding manual run mode I will merge it 🎉 Thanks @donaldpipowitch 👏

And some technical improvements can be done in another PR, for instance:

use useAddonState
check if active flag can be removed as indicated in a comment

donaldpipowitch · 2020-01-14T06:15:05Z

Thank you so much 😊

donaldpipowitch requested review from CodeByAlex, jbovenschen, ndelangen and tmeasday as code owners November 19, 2019 09:34

vercel bot deployed to staging November 19, 2019 09:34 View deployment

CodeByAlex reviewed Nov 19, 2019

View reviewed changes

ndelangen added addon: a11y feature request labels Nov 19, 2019

ndelangen assigned donaldpipowitch Nov 19, 2019

shilman changed the title ~~addon-a11y: allow manual run~~ Addon-a11y: allow manual run Nov 20, 2019

shilman changed the title ~~Addon-a11y: allow manual run~~ Addon-a11y: Enable manual run Nov 20, 2019

shilman changed the title ~~Addon-a11y: Enable manual run~~ Addon-a11y: Support manual run Nov 20, 2019

donaldpipowitch force-pushed the 8126-addon-a11y--allow-manual-run branch from 229229a to 56ddeaf Compare November 25, 2019 14:24

vercel bot deployed to staging November 25, 2019 14:24 View deployment

vercel bot deployed to staging November 25, 2019 20:16 View deployment

donaldpipowitch removed their assignment Nov 26, 2019

donaldpipowitch requested a review from CodeByAlex November 26, 2019 05:52

donaldpipowitch added 2 commits December 2, 2019 10:02

addon-a11y: allow manual run

d73733d

updated tests

aa5a654

donaldpipowitch force-pushed the 8126-addon-a11y--allow-manual-run branch from 744fc2f to aa5a654 Compare December 2, 2019 09:03

vercel bot had a problem deploying to staging December 2, 2019 09:03 Failure

Merge branch 'next' into 8126-addon-a11y--allow-manual-run

268cdb2

vercel bot deployed to staging December 6, 2019 12:20 View deployment

ndelangen added this to the 5.4.0 milestone Dec 6, 2019

shilman removed this from the 5.4.0 milestone Dec 16, 2019

donaldpipowitch requested review from dangreenisrael, igor-dv, thani-sh and theinterned as code owners December 21, 2019 20:09

vercel bot deployed to staging December 21, 2019 20:09 View deployment

ndelangen changed the base branch from next to next-6.0.0 December 21, 2019 20:14

gaetanmaisse mentioned this pull request Dec 24, 2019

Use TS strict mode in a11y addon sources #9180

Merged

gaetanmaisse reviewed Dec 31, 2019

View reviewed changes

gaetanmaisse self-assigned this Jan 11, 2020

Merge branch 'next-6.0.0' into 8126-addon-a11y--allow-manual-run

e368e57

vercel bot deployed to Preview January 12, 2020 19:48 View deployment

gaetanmaisse reviewed Jan 12, 2020

View reviewed changes

donaldpipowitch added 2 commits January 13, 2020 08:36

fix initial render bug

a03a2c4

Merge branch '8126-addon-a11y--allow-manual-run' of github.com:donald…

f567dad

…pipowitch/storybook into 8126-addon-a11y--allow-manual-run

vercel bot deployed to Preview January 13, 2020 07:39 View deployment

gaetanmaisse approved these changes Jan 13, 2020

View reviewed changes

gaetanmaisse merged commit f486faa into storybookjs:next-6.0.0 Jan 13, 2020

shilman mentioned this pull request Jan 21, 2020

a11y-addon: Parameters at story level affect other stories #8126

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Addon-a11y: Support manual run #8883

Addon-a11y: Support manual run #8883

donaldpipowitch commented Nov 19, 2019 •

edited

Loading

vercel bot commented Nov 19, 2019 •

edited

Loading

CodeByAlex Nov 19, 2019

donaldpipowitch Nov 21, 2019 •

edited

Loading

ndelangen Dec 6, 2019

donaldpipowitch Dec 16, 2019

donaldpipowitch commented Nov 25, 2019

donaldpipowitch commented Dec 2, 2019

ndelangen commented Dec 6, 2019

donaldpipowitch commented Dec 6, 2019

ndelangen commented Dec 21, 2019

donaldpipowitch commented Dec 22, 2019

gaetanmaisse Dec 31, 2019

donaldpipowitch Jan 8, 2020

ndelangen Jan 8, 2020 •

edited

Loading

ndelangen Jan 8, 2020

donaldpipowitch Jan 9, 2020 •

edited

Loading

gaetanmaisse Jan 9, 2020

ndelangen commented Jan 11, 2020

gaetanmaisse commented Jan 11, 2020

donaldpipowitch commented Jan 12, 2020

gaetanmaisse Jan 12, 2020

gaetanmaisse Jan 12, 2020

donaldpipowitch Jan 13, 2020

gaetanmaisse Jan 13, 2020

gaetanmaisse commented Jan 13, 2020

donaldpipowitch commented Jan 14, 2020

Addon-a11y: Support manual run #8883

Addon-a11y: Support manual run #8883

Conversation

donaldpipowitch commented Nov 19, 2019 • edited Loading

What I did

How to test

vercel bot commented Nov 19, 2019 • edited Loading

Choose a reason for hiding this comment

donaldpipowitch Nov 21, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

donaldpipowitch commented Nov 25, 2019

donaldpipowitch commented Dec 2, 2019

ndelangen commented Dec 6, 2019

donaldpipowitch commented Dec 6, 2019

ndelangen commented Dec 21, 2019

donaldpipowitch commented Dec 22, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ndelangen Jan 8, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

donaldpipowitch Jan 9, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ndelangen commented Jan 11, 2020

gaetanmaisse commented Jan 11, 2020

donaldpipowitch commented Jan 12, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gaetanmaisse commented Jan 13, 2020

donaldpipowitch commented Jan 14, 2020

donaldpipowitch commented Nov 19, 2019 •

edited

Loading

vercel bot commented Nov 19, 2019 •

edited

Loading

donaldpipowitch Nov 21, 2019 •

edited

Loading

ndelangen Jan 8, 2020 •

edited

Loading

donaldpipowitch Jan 9, 2020 •

edited

Loading