-
-
Notifications
You must be signed in to change notification settings - Fork 9.4k
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 discrepancy handling to A11yPanel #29661
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,6 +8,7 @@ import { CheckIcon, SyncIcon } from '@storybook/icons'; | |
import { useA11yContext } from './A11yContext'; | ||
import { Report } from './Report'; | ||
import { Tabs } from './Tabs'; | ||
import { TestDiscrepancyMessage } from './TestDiscrepancyMessage'; | ||
|
||
export enum RuleType { | ||
VIOLATION, | ||
|
@@ -43,7 +44,7 @@ const Centered = styled.span({ | |
}); | ||
|
||
export const A11YPanel: React.FC = () => { | ||
const { results, status, handleManual, error } = useA11yContext(); | ||
const { results, status, handleManual, error, discrepancy } = useA11yContext(); | ||
|
||
const manualActionItems = useMemo( | ||
() => [{ title: 'Run test', onClick: handleManual }], | ||
|
@@ -106,31 +107,35 @@ export const A11YPanel: React.FC = () => { | |
|
||
return ( | ||
<> | ||
{status === 'initial' && <Centered>Initializing...</Centered>} | ||
{status === 'manual' && ( | ||
<> | ||
<Centered>Manually run the accessibility scan.</Centered> | ||
<ActionBar key="actionbar" actionItems={manualActionItems} /> | ||
</> | ||
)} | ||
{status === 'running' && ( | ||
<Centered> | ||
<RotatingIcon size={12} /> Please wait while the accessibility scan is running ... | ||
</Centered> | ||
)} | ||
{(status === 'ready' || status === 'ran') && ( | ||
{discrepancy && <TestDiscrepancyMessage discrepancy={discrepancy} />} | ||
{status === 'ready' || status === 'ran' ? ( | ||
<> | ||
<ScrollArea vertical horizontal> | ||
<Tabs key="tabs" tabs={tabs} /> | ||
</ScrollArea> | ||
<ActionBar key="actionbar" actionItems={readyActionItems} /> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. style: key prop on ActionBar is unnecessary since it's not in an array/iteration |
||
</> | ||
)} | ||
{status === 'error' && ( | ||
<Centered> | ||
The accessibility scan encountered an error. | ||
<br /> | ||
{typeof error === 'string' ? error : JSON.stringify(error)} | ||
) : ( | ||
<Centered style={{ marginTop: discrepancy ? '1em' : 0 }}> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. style: Consider extracting inline style to a styled component for consistency with the rest of the codebase |
||
{status === 'initial' && 'Initializing...'} | ||
{status === 'manual' && ( | ||
<> | ||
<>Manually run the accessibility scan.</> | ||
<ActionBar key="actionbar" actionItems={manualActionItems} /> | ||
Comment on lines
+122
to
+124
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. syntax: Unnecessary nested fragment (<>) inside another fragment. Remove the inner fragment. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. style: key prop on ActionBar is unnecessary since it's not in an array/iteration |
||
</> | ||
)} | ||
{status === 'running' && ( | ||
<> | ||
<RotatingIcon size={12} /> Please wait while the accessibility scan is running ... | ||
</> | ||
)} | ||
{status === 'error' && ( | ||
<> | ||
The accessibility scan encountered an error. | ||
<br /> | ||
{typeof error === 'string' ? error : JSON.stringify(error)} | ||
</> | ||
)} | ||
</Centered> | ||
)} | ||
</> | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,73 @@ | ||
import React, { useMemo } from 'react'; | ||
|
||
import { Link } from 'storybook/internal/components'; | ||
import { useStorybookApi } from 'storybook/internal/manager-api'; | ||
import { styled } from 'storybook/internal/theming'; | ||
|
||
import { DOCUMENTATION_DISCREPANCY_LINK } from '../constants'; | ||
|
||
const Wrapper = styled.div(({ theme: { color, typography, background } }) => ({ | ||
textAlign: 'start', | ||
padding: '11px 15px', | ||
fontSize: `${typography.size.s2}px`, | ||
fontWeight: typography.weight.regular, | ||
lineHeight: '1rem', | ||
background: background.app, | ||
borderBottom: `1px solid ${color.border}`, | ||
color: color.defaultText, | ||
backgroundClip: 'padding-box', | ||
position: 'relative', | ||
code: { | ||
fontSize: `${typography.size.s1 - 1}px`, | ||
color: 'inherit', | ||
margin: '0 0.2em', | ||
padding: '0 0.2em', | ||
background: 'rgba(255, 255, 255, 0.8)', | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. style: background color with alpha channel may not provide sufficient contrast with dark themes |
||
borderRadius: '2px', | ||
boxShadow: '0 0 0 1px rgba(0, 0, 0, 0.1)', | ||
}, | ||
})); | ||
|
||
export type TestDiscrepancy = | ||
| 'browserPassedCliFailed' | ||
| 'cliPassedBrowserFailed' | ||
| 'cliFailedButModeManual' | ||
| null; | ||
|
||
interface TestDiscrepancyMessageProps { | ||
discrepancy: TestDiscrepancy; | ||
} | ||
export const TestDiscrepancyMessage = ({ discrepancy }: TestDiscrepancyMessageProps) => { | ||
const api = useStorybookApi(); | ||
const docsUrl = api.getDocsUrl({ | ||
subpath: DOCUMENTATION_DISCREPANCY_LINK, | ||
versioned: true, | ||
renderer: true, | ||
}); | ||
|
||
const message = useMemo(() => { | ||
switch (discrepancy) { | ||
case 'browserPassedCliFailed': | ||
return 'Accessibility checks passed in this browser but failed in the CLI.'; | ||
case 'cliPassedBrowserFailed': | ||
return 'Accessibility checks passed in the CLI but failed in this browser.'; | ||
case 'cliFailedButModeManual': | ||
return 'Accessibility checks failed in the CLI. Run the tests manually to see the results.'; | ||
default: | ||
return null; | ||
} | ||
}, [discrepancy]); | ||
|
||
if (!message) { | ||
return null; | ||
} | ||
|
||
return ( | ||
<Wrapper> | ||
{message}{' '} | ||
<Link href={docsUrl} target="_blank" withArrow> | ||
Learn what could cause this | ||
</Link> | ||
</Wrapper> | ||
); | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,4 +7,9 @@ const RUNNING = `${ADDON_ID}/running`; | |
const ERROR = `${ADDON_ID}/error`; | ||
const MANUAL = `${ADDON_ID}/manual`; | ||
|
||
export const DOCUMENTATION_LINK = 'writing-tests/accessibility-testing'; | ||
export const DOCUMENTATION_DISCREPANCY_LINK = `${DOCUMENTATION_LINK}#what-happens-when-there-are-different-results-in-multiple-environments`; | ||
Comment on lines
+10
to
+11
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. style: consider making this a full URL path rather than a relative path to ensure it works across different contexts |
||
|
||
export const TEST_PROVIDER_ID = 'storybook/addon-a11y/test-provider'; | ||
|
||
export const EVENTS = { RESULT, REQUEST, RUNNING, ERROR, MANUAL }; |
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.
style: key prop on Tabs is unnecessary since it's not in an array/iteration