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

Add an escape hatch for a11y errors #1083

Merged
merged 3 commits into from
Aug 5, 2021
Merged

Conversation

robinmetral
Copy link
Contributor

Purpose

#1080 introduced a mechanism that throws at missing accessible labels across all components.

However, this would require large-scale change in some applications, so we're also providing an escape hatch to allow developers to work on their applications while labels are still not provided, for example while they're being written and localized.

The errors will not be thrown if the UNSAFE_DISABLE_ACCESSIBILITY_ERRORS environment variable is set to true.

Note that depending on the application, this environment variable will need to be exposed differently.

In a Next.js application, it can be added in the next.config.js:

module.exports = {
  // ...
  env: {
    // ...
    UNSAFE_DISABLE_ACCESSIBILITY_ERRORS: process.env.UNSAFE_DISABLE_ACCESSIBILITY_ERRORS,
  },
};

In other applications, the Webpack config can be extended with the Webpack DefinePlugin:

(note: this is the approach this PR is taking for the Circuit UI Storybook)

const webpack = require('webpack');
module.exports = {
  plugins = [
    new webpack.DefinePlugin({
      'process.env.UNSAFE_DISABLE_ACCESSIBILITY_ERRORS': JSON.stringify(
        process.env.UNSAFE_DISABLE_ACCESSIBILITY_ERRORS,
      ),
    }),
  ];
};

Then, the variable can be set in the console to bypass accessibility errors in development:

UNSAFE_DISABLE_ACCESSIBILITY_ERRORS=true yarn dev

Approach and changes

  • add a check for the UNSAFE_DISABLE_ACCESSIBILITY_ERRORS environment variable before throwing when labels are missing in development
  • extend Storybook Webpack plugin to accept the environment variable from the console (for local testing mostly, but this escape hatch should obviously not be used in Circuit)

Definition of done

  • Development completed
  • Reviewers assigned
  • [ ] Unit and integration tests
  • [ ] Meets minimum browser support
  • [ ] Meets accessibility requirements

Robin Métral added 2 commits August 4, 2021 17:56
This commit adds the condition to all files where the error would be thrown and configures the Storybook to also receive the environment variable from the CLI
@robinmetral robinmetral requested a review from a team as a code owner August 4, 2021 16:14
@robinmetral robinmetral requested review from connor-baer and removed request for a team August 4, 2021 16:14
@changeset-bot
Copy link

changeset-bot bot commented Aug 4, 2021

🦋 Changeset detected

Latest commit: 82cb833

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@sumup/circuit-ui Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@vercel
Copy link

vercel bot commented Aug 4, 2021

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

🔍 Inspect: https://vercel.com/sumup/oss-circuit-ui/4zCdsTG66dYByCS1M4qd4cDUddBo
✅ Preview: https://oss-circuit-ui-git-a11y-errors-escape-hatch-sumup.vercel.app

@sumup-clark
Copy link

sumup-clark bot commented Aug 4, 2021

Hey @robinmetral,
we are super excited that you are contributing! 🎉There's one more thing you need to do. Please accept our Contributor License Agreement. It helps you and us to collaborate on clear terms and focus on what we love most: code.

Thanks!

@codecov
Copy link

codecov bot commented Aug 4, 2021

Codecov Report

Merging #1083 (82cb833) into next (c1a00e3) will increase coverage by 0.05%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             next    #1083      +/-   ##
==========================================
+ Coverage   91.54%   91.59%   +0.05%     
==========================================
  Files         165      165              
  Lines        2908     2926      +18     
  Branches      855      873      +18     
==========================================
+ Hits         2662     2680      +18     
  Misses        233      233              
  Partials       13       13              
Impacted Files Coverage Δ
...ages/circuit-ui/components/Hamburger/Hamburger.tsx 94.28% <100.00%> (+0.16%) ⬆️
...es/circuit-ui/components/IconButton/IconButton.tsx 88.23% <100.00%> (+0.73%) ⬆️
...es/circuit-ui/components/ImageInput/ImageInput.tsx 85.71% <100.00%> (+0.15%) ⬆️
packages/circuit-ui/components/Input/Input.tsx 93.44% <100.00%> (+0.10%) ⬆️
...cuit-ui/components/LoadingButton/LoadingButton.tsx 90.90% <100.00%> (+0.43%) ⬆️
packages/circuit-ui/components/Modal/Modal.tsx 81.57% <100.00%> (+0.49%) ⬆️
...ircuit-ui/components/ModalContext/ModalContext.tsx 87.23% <100.00%> (+0.27%) ⬆️
...es/circuit-ui/components/Pagination/Pagination.tsx 91.42% <100.00%> (+0.25%) ⬆️
.../circuit-ui/components/ProgressBar/ProgressBar.tsx 95.65% <100.00%> (+0.09%) ⬆️
...i/components/RadioButtonGroup/RadioButtonGroup.tsx 89.47% <100.00%> (+0.58%) ⬆️
... and 8 more

Comment on lines +169 to 172
process.env.UNSAFE_DISABLE_ACCESSIBILITY_ERRORS !== 'true' &&
process.env.NODE_ENV !== 'production' &&
process.env.NODE_ENV !== 'test' &&
(!activeLabel || !inactiveLabel)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wondering if there's a way to abstract this away somewhere to avoid 3 duplicate lines in 30ish components

Copy link
Member

Choose a reason for hiding this comment

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

I don't think there is a way without adding a function call in production everywhere 😕

@@ -40,5 +42,14 @@ function transpileModules(config) {
}
return rule;
});
// Expose environment variables to Storybook
config.plugins = [
...config.plugins,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure if this is necessary. I thought it couldn't hurt

config.plugins = [
...config.plugins,
new webpack.DefinePlugin({
'process.env.UNSAFE_DISABLE_ACCESSIBILITY_ERRORS': JSON.stringify(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note: here I had to use JSON.stringify() as mentioned in the plugin's docs, but in the dashboard's Next.js config I didn't need to (doing so resulted in UNSAFE_DISABLE_ACCESSIBILITY_ERRORS === '"true"'), I assume Next.js is doing it under the hood

Copy link
Member

@connor-baer connor-baer left a comment

Choose a reason for hiding this comment

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

This is great! 🙌🏻

Comment on lines +169 to 172
process.env.UNSAFE_DISABLE_ACCESSIBILITY_ERRORS !== 'true' &&
process.env.NODE_ENV !== 'production' &&
process.env.NODE_ENV !== 'test' &&
(!activeLabel || !inactiveLabel)
Copy link
Member

Choose a reason for hiding this comment

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

I don't think there is a way without adding a function call in production everywhere 😕

@robinmetral robinmetral merged commit 3513326 into next Aug 5, 2021
@robinmetral robinmetral deleted the a11y-errors-escape-hatch branch August 5, 2021 05:43
@amelako amelako mentioned this pull request Apr 6, 2022
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants