Addon-a11y: Support manual run #8883
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,8 +1,8 @@ | ||
| /* eslint-disable react/destructuring-assignment,default-case,consistent-return,no-case-declarations */ | ||
| import React, { Component, Fragment } from 'react'; | ||
|
|
||
| import { styled } from '@storybook/theming'; | ||
|
|
||
| import { ActionBar, Icons, ScrollArea } from '@storybook/components'; | ||
|
|
||
| import { AxeResults, Result } from 'axe-core'; | ||
|
|
@@ -20,60 +20,70 @@ export enum RuleType { | |
| INCOMPLETION, | ||
| } | ||
|
|
||
| const RotatingIcons = styled(Icons)(({ theme }) => ({ | ||
| height: '12px', | ||
| width: '12px', | ||
| marginRight: '4px', | ||
| animation: `${theme.animation.rotate360} 1s linear infinite;`, | ||
| })); | ||
|
|
||
| const Passes = styled.span(({ theme }) => ({ | ||
| color: theme.color.positive, | ||
| })); | ||
|
|
||
| const Violations = styled.span(({ theme }) => ({ | ||
| color: theme.color.negative, | ||
| })); | ||
|
|
||
| const Incomplete = styled.span(({ theme }) => ({ | ||
| color: theme.color.warning, | ||
| })); | ||
|
|
||
| const Centered = styled.span({ | ||
| display: 'flex', | ||
| alignItems: 'center', | ||
| justifyContent: 'center', | ||
| height: '100%', | ||
| }); | ||
|
|
||
| interface InitialState { | ||
| status: 'initial'; | ||
| } | ||
|
|
||
| interface ManualState { | ||
| status: 'manual'; | ||
| } | ||
|
|
||
| interface RunningState { | ||
| status: 'running'; | ||
| } | ||
|
|
||
| interface RanState { | ||
| status: 'ran'; | ||
| passes: Result[]; | ||
| violations: Result[]; | ||
| incomplete: Result[]; | ||
| } | ||
|
|
||
| interface ReadyState { | ||
| status: 'ready'; | ||
| passes: Result[]; | ||
| violations: Result[]; | ||
| incomplete: Result[]; | ||
| } | ||
|
|
||
| interface ErrorState { | ||
| status: 'error'; | ||
| error: unknown; | ||
| } | ||
|
|
||
| type A11YPanelState = | ||
| | InitialState | ||
| | ManualState | ||
| | RunningState | ||
| | RanState | ||
| | ReadyState | ||
| | ErrorState; | ||
|
|
||
| interface A11YPanelProps { | ||
| active: boolean; | ||
|
|
@@ -82,18 +92,15 @@ interface A11YPanelProps { | |
|
|
||
| export class A11YPanel extends Component<A11YPanelProps, A11YPanelState> { | ||
| state: A11YPanelState = { | ||
| status: 'initial', | ||
| }; | ||
|
|
||
| componentDidMount() { | ||
| const { api } = this.props; | ||
|
|
||
| api.on(EVENTS.RESULT, this.onResult); | ||
| api.on(EVENTS.ERROR, this.onError); | ||
| api.on(EVENTS.MANUAL, this.onManual); | ||
| } | ||
|
|
||
| componentDidUpdate(prevProps: A11YPanelProps) { | ||
|
|
@@ -103,18 +110,18 @@ export class A11YPanel extends Component<A11YPanelProps, A11YPanelState> { | |
| if (!prevProps.active && active) { | ||
| // removes all elements from the redux map in store from the previous panel | ||
| store.dispatch(clearElements()); | ||
| } | ||
| } | ||
|
|
||
| componentWillUnmount() { | ||
| const { api } = this.props; | ||
|
|
||
| api.off(EVENTS.RESULT, this.onResult); | ||
| api.off(EVENTS.ERROR, this.onError); | ||
| api.off(EVENTS.MANUAL, this.onManual); | ||
| } | ||
|
|
||
| onResult = ({ passes, violations, incomplete }: AxeResults) => { | ||
| this.setState( | ||
| { | ||
| status: 'ran', | ||
|
|
@@ -142,9 +149,18 @@ export class A11YPanel extends Component<A11YPanelProps, A11YPanelState> { | |
| }); | ||
| }; | ||
|
|
||
| onManual = (manual: boolean) => { | ||
| if (manual) { | ||
| this.setState({ | ||
| status: 'manual', | ||
| }); | ||
| } else { | ||
| this.request(); | ||
|
There was a problem hiding this comment. Are we sure that the story has been rendered before calling There was a problem hiding this comment. It seemed to work for me without problems, but I don't know the internals. Maybe @ndelangen could confirm if events emitted inside There was a problem hiding this comment. STORY_RENDERED is called after the awaited render function resolved/returned: There was a problem hiding this comment. So I do believe it's called later. Whether or not this impacts the a11y execution, I don't know. There was a problem hiding this comment. There actually seems to be a problem - not sure if it's because of the missing 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 😭 There was a problem hiding this comment. Good catch @donaldpipowitch, yes we need to fix this, I will try to take a look tonight if I have the time ;) |
||
| } | ||
| }; | ||
|
|
||
| request = () => { | ||
|
There was a problem hiding this comment. @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 There was a problem hiding this comment. @gaetanmaisse thank you for having a look. This seems to fix the issue - it now works for me locally. I'm unsure about For now I just added your fix and kept other usages of There was a problem hiding this comment. I tried to simply remove all |
||
| const { api, active } = this.props; | ||
| if (active) { | ||
| this.setState( | ||
| { | ||
|
|
@@ -163,43 +179,39 @@ export class A11YPanel extends Component<A11YPanelProps, A11YPanelState> { | |
| const { active } = this.props; | ||
| if (!active) return null; | ||
|
|
||
| switch (this.state.status) { | ||
| case 'initial': | ||
| return <Centered>Initializing...</Centered>; | ||
| case 'manual': | ||
| return ( | ||
| <Fragment> | ||
| <Centered>Manually run the accessibility scan.</Centered> | ||
| <ActionBar | ||
| key="actionbar" | ||
| actionItems={[{ title: 'Run test', onClick: this.request }]} | ||
| /> | ||
| </Fragment> | ||
| ); | ||
| case 'running': | ||
| return ( | ||
| <Centered> | ||
| <RotatingIcons inline icon="sync" /> Please wait while the accessibility scan is running | ||
| ... | ||
| </Centered> | ||
| ); | ||
| case 'ready': | ||
| case 'ran': | ||
| const { passes, violations, incomplete, status } = this.state; | ||
| const actionTitle = | ||
| status === 'ready' ? ( | ||
| 'Rerun tests' | ||
| ) : ( | ||
| <Fragment> | ||
| <Icons inline icon="check" /> Tests completed | ||
| </Fragment> | ||
| ); | ||
| return ( | ||
| <Provider store={store}> | ||
| <ScrollArea vertical horizontal> | ||
| <Tabs | ||
| key="tabs" | ||
|
|
@@ -243,13 +255,21 @@ export class A11YPanel extends Component<A11YPanelProps, A11YPanelState> { | |
| ]} | ||
| /> | ||
| </ScrollArea> | ||
| <ActionBar | ||
| key="actionbar" | ||
| actionItems={[{ title: actionTitle, onClick: this.request }]} | ||
| /> | ||
| </Provider> | ||
| ); | ||
| case 'error': | ||
| const { error } = this.state; | ||
| return ( | ||
| <Centered> | ||
| The accessibility scan encountered an error. | ||
| <br /> | ||
| {error} | ||
| </Centered> | ||
| ); | ||
| } | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couldn’t these 4 be made into a single interface with options for status? In other words, extending the old interface.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
readystate 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,manualandrunningstates. 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 (likeerror).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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Long term, this addon should start using the
useAddonStatetbh.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ndelangen Sorry, just saw your comment now. Did I understand you correctly that the approach is fine for now and
useAddonStateshould be used in a future PR? Or should I adduseAddonStateto this PR?