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

docs(storybook): fixes a number of accessibility issues in the documentation #7021

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

damienrobson-sage
Copy link
Contributor

Proposed behaviour

The documentation offered by the storybook should be as accessible as possible. This PR aims to implement that by addressing a handful of issues:

  • Changes the default secondary colour of the manager to the DS' Sage Green
  • Fix the role assigned to the welcome page's headings
  • Remove the duplicated header elements
  • Add ARIA labels
  • Add image alt text
  • Replace blank table headings

Current behaviour

A wide range of stories feature AXE failings, which results in a poor documentation experience for users who e.g. rely on assistive technologies.

Checklist

  • Commits follow our style guide
  • Related issues linked in commit messages if required
  • Screenshots are included in the PR if useful
  • All themes are supported if required
  • Unit tests added or updated if required
  • Playwright automation tests added or updated if required
  • Storybook added or updated if required
  • Translations added or updated (including creating or amending translation keys table in storybook) if required
  • Typescript d.ts file added or updated if required
  • Related docs have been updated if required

QA

  • Tested in provided StackBlitz sandbox/Storybook
  • Add new Playwright test coverage if required
  • Carbon implementation matches Design System/designs
  • UI Tests GitHub check reviewed if required

Additional context

The following issues are embedded within Storybook and can't be directly addressed:

  • The Create a new story button throws a "Buttons must have discernible text" warning (only in localhost, live build is fine);
  • The "Zooming and scaling" failure is due to Storybook's meta tag configuration which we can't override (so I've raised an issue with them to fix it)
  • The accessibility control panel causes 4 landmark violations

Testing instructions

Install Axe DevTools if not already installed. Open the documentation and then open the browser's inspector and switch to the DevTools tab. Run a full-page scan.

@damienrobson-sage
Copy link
Contributor Author

Outstanding issues:

Cannot be directly addressed:

  • Code blocks: variable and property names do not satisfy minimum colour contrast ratios of 4.5:1
  • Storybook: zooming and scaling disabled
  • Storybook: create new story button in development mode has no discernible text

Will require DS input et. al.:

  • BatchSelection > selection count > minimum colour contrast of labels does not meet 4.5:1
  • ButtonToggle > Disabled Group > minimum colour contrast of labels does not meet 4.5:1
  • Link > Disabled > minimum colour contrast of labels does not meet 4.5:1
  • Link > Dark background > minimum colour contrast of labels does not meet 4.5:1
  • Pill > Custom colours > example red and orange variants do not satisfy minimum colour contrast ratios of 4.5:1
  • Search > Search with Alternative Styling and No Button > minimum colour contrast of placeholder text does not meet 4.5:1
  • Select > Disabled > minimum colour contrast of selected option text does not meet 4.5:1
  • TileSelect > Disabled tiles > minimum colour contrast of disabled tile text does not meet 4.5:1
  • Tile > with TileFooter > ternary button texts do not meet minimum colour thresholds
  • Time > Disabled > minimum colour contrast of disabled text does not meet 4.5:1

The Heading component also flags heading level order violations but these are:

  • Best Practice suggestions, and therefore not essential;
  • expected given the nature of the story.

@tomdavies73 tomdavies73 self-requested a review October 16, 2024 09:24
nineteen88
nineteen88 previously approved these changes Oct 16, 2024
@nineteen88
Copy link
Contributor

Looks good @damienrobson-sage. I've changed the label to be pending UX QA as I'd like @harpalsingh to have a look at some bits. Namely we've changed the colour of the highlight and selected states on the left sidebar menu. This is probably fine, but we needed a general look at the general aesthetic of Storybook anyway, so he might as well do it as part of this. Likelihood is we will just create tasks outside of this PR to be addressed, and not require any further changes (unless there's a specific case that makes sense in this PR).

tomdavies73
tomdavies73 previously approved these changes Oct 16, 2024
Copy link
Contributor

@tomdavies73 tomdavies73 left a comment

Choose a reason for hiding this comment

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

Good work on this @damienrobson-sage, I agree with @nineteen88 that getting a UX perspective on these changes would be beneficial.

I'm going to approve, but also thought it was mentioning that it could be a good idea to convert some of these js files to ts to match the rest of the codebase. However, I understand it is not really in scope with these changes. So happy to leave it and approve as is 👍

@damienrobson-sage
Copy link
Contributor Author

@tomdavies73 I'll leave them as JS for now to avoid scope creep - probably not a good habit to get into on my first ticket 😆

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

5 participants