Skip to content

Add discrepancy handling to A11yPanel#29661

Merged
valentinpalkovic merged 2 commits into
valentin/unified-a11y-testingfrom
valentin/add-a11y-discrepancy-handling
Nov 20, 2024
Merged

Add discrepancy handling to A11yPanel#29661
valentinpalkovic merged 2 commits into
valentin/unified-a11y-testingfrom
valentin/add-a11y-discrepancy-handling

Conversation

@valentinpalkovic
Copy link
Copy Markdown
Contributor

@valentinpalkovic valentinpalkovic commented Nov 19, 2024

Closes #

What I did

Checklist for Contributors

Testing

The changes in this PR are covered in the following automated tests:

  • stories
  • unit tests
  • integration tests
  • end-to-end tests

Manual testing

This section is mandatory for all contributions. If you believe no manual test is necessary, please state so explicitly. Thanks!

Documentation

  • Add or update documentation reflecting your changes
  • If you are deprecating/removing a feature, make sure to update
    MIGRATION.MD

Checklist for Maintainers

  • When this PR is ready for testing, make sure to add ci:normal, ci:merged or ci:daily GH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found in code/lib/cli-storybook/src/sandbox-templates.ts

  • Make sure this PR contains one of the labels below:

    Available labels
    • bug: Internal changes that fixes incorrect behavior.
    • maintenance: User-facing maintenance tasks.
    • dependencies: Upgrading (sometimes downgrading) dependencies.
    • build: Internal-facing build tooling & test updates. Will not show up in release changelog.
    • cleanup: Minor cleanup style change. Will not show up in release changelog.
    • documentation: Documentation only changes. Will not show up in release changelog.
    • feature request: Introducing a new feature.
    • BREAKING CHANGE: Changes that break compatibility in some way with current major version.
    • other: Changes that don't fit in the above categories.

🦋 Canary release

This pull request has been released as version 0.0.0-pr-29661-sha-2eebee12. Try it out in a new sandbox by running npx storybook@0.0.0-pr-29661-sha-2eebee12 sandbox or in an existing project with npx storybook@0.0.0-pr-29661-sha-2eebee12 upgrade.

More information
Published version 0.0.0-pr-29661-sha-2eebee12
Triggered by @valentinpalkovic
Repository storybookjs/storybook
Branch valentin/add-a11y-discrepancy-handling
Commit 2eebee12
Datetime Tue Nov 19 15:05:09 UTC 2024 (1732028709)
Workflow run 11915852041

To request a new release of this pull request, mention the @storybookjs/core team.

core team members can create a new canary release here or locally with gh workflow run --repo storybookjs/storybook canary-release-pr.yml --field pr=29661

Greptile Summary

Added discrepancy handling to the A11YPanel component to detect and display messages when accessibility test results differ between CLI and browser environments.

  • Added new TestDiscrepancyMessage component in /code/addons/a11y/src/components/TestDiscrepancyMessage.tsx to display environment-specific test result differences
  • Added discrepancy field to A11yContext in /code/addons/a11y/src/components/A11yContext.tsx to track test result mismatches
  • Added comprehensive test coverage in /code/addons/a11y/src/components/A11yContext.test.tsx for discrepancy scenarios
  • Added documentation links in /code/addons/a11y/src/constants.ts for explaining discrepancy cases
  • Added new stories in /code/addons/a11y/template/stories/TestDiscrepancyMessage.stories.tsx to showcase different discrepancy states

@valentinpalkovic valentinpalkovic self-assigned this Nov 19, 2024
@valentinpalkovic valentinpalkovic added accessibility ci:normal Run our default set of CI jobs (choose this for most PRs). feature request labels Nov 19, 2024
@nx-cloud
Copy link
Copy Markdown

nx-cloud Bot commented Nov 19, 2024

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 79ad98c. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this CI Pipeline Execution


✅ Successfully ran 1 target

Sent with 💌 from NxCloud.

@valentinpalkovic valentinpalkovic marked this pull request as ready for review November 20, 2024 09:57
Copy link
Copy Markdown
Contributor

@greptile-apps greptile-apps Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

8 file(s) reviewed, 9 comment(s)
Edit PR Review Bot Settings | Greptile

Comment on lines +122 to +124
<>
<>Manually run the accessibility scan.</>
<ActionBar key="actionbar" actionItems={manualActionItems} />
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

syntax: Unnecessary nested fragment (<>) inside another fragment. Remove the inner fragment.

{status === 'ready' || status === 'ran' ? (
<>
<ScrollArea vertical horizontal>
<Tabs key="tabs" tabs={tabs} />
Copy link
Copy Markdown
Contributor

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

<ScrollArea vertical horizontal>
<Tabs key="tabs" tabs={tabs} />
</ScrollArea>
<ActionBar key="actionbar" actionItems={readyActionItems} />
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The 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 === 'manual' && (
<>
<>Manually run the accessibility scan.</>
<ActionBar key="actionbar" actionItems={manualActionItems} />
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The 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

<br />
{typeof error === 'string' ? error : JSON.stringify(error)}
) : (
<Centered style={{ marginTop: discrepancy ? '1em' : 0 }}>
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The 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

color: 'inherit',
margin: '0 0.2em',
padding: '0 0.2em',
background: 'rgba(255, 255, 255, 0.8)',
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The 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

Comment on lines +10 to +11
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`;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The 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

Comment on lines +15 to +20
const managerContext: any = {
state: {},
api: {
getDocsUrl: fn().mockName('api::getDocsUrl'),
},
};
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style: managerContext is typed as 'any' - consider creating a proper type for this mock object

Comment on lines +12 to +17
const managerContext: any = {
state: {},
api: {
getDocsUrl: fn().mockName('api::getDocsUrl'),
},
};
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style: managerContext type should be properly defined instead of using 'any'

@valentinpalkovic valentinpalkovic merged commit 6dfea64 into valentin/unified-a11y-testing Nov 20, 2024
@valentinpalkovic valentinpalkovic deleted the valentin/add-a11y-discrepancy-handling branch November 20, 2024 10:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

accessibility ci:normal Run our default set of CI jobs (choose this for most PRs). feature request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants