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

feat(loader): remove progressbar role and add loaderLabel prop #7176

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

nuria1110
Copy link
Contributor

@nuria1110 nuria1110 commented Jan 23, 2025

fix #6956

Proposed behaviour

  • Remove role of "progressbar".
  • Deprecate aria-label prop.
  • Add loaderLabel prop to set a visually hidden label for the component instead of an aria-label.

Current behaviour

Loader has role of "progressbar".

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

Testing instructions

edleeks87
edleeks87 previously approved these changes Jan 24, 2025

Please note that ARIA live regions provide a means for conveying dynamic updates to users who rely on assistive technologies like screen readers.
These updates in this case include changes to the button's content. By incorporating live regions permanently into the DOM, we can ensure that users are consistently informed of relevant changes as they occur.
Note that `Loader` has a role of "status", meaning it has an implicit `aria-live="polite"` and will be announced by screen readers.
Copy link
Contributor

Choose a reason for hiding this comment

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

🔥

<Button
m={2}
buttonType="primary"
aria-label={ariaContent}
Copy link
Contributor

Choose a reason for hiding this comment

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

Question (non-blocking): Does this need an aria-label at all when isLoading is false as there's text content at that point?

  const ariaContent = isLoading ? "Loading" : undefined;

@Parsium Parsium self-requested a review January 24, 2025 09:14
@nuria1110 nuria1110 changed the title fix(loader): set role to status feat(loader): remove progressbar role and add loaderLabel prop Jan 24, 2025
src/components/confirm/confirm.pw.tsx Outdated Show resolved Hide resolved
src/components/loader/loader.component.tsx Outdated Show resolved Hide resolved
src/components/loader/loader.component.tsx Outdated Show resolved Hide resolved

### Conditional Rendering

In most cases, Loaders are conditionally rendered on the page and are then replaced by content.
Copy link
Contributor

Choose a reason for hiding this comment

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

praise: this is much simpler now 👍🏼

src/components/loader/loader.stories.tsx Outdated Show resolved Hide resolved
src/components/loader/loader.test.tsx Outdated Show resolved Hide resolved
Removes role of progressbar in Loader as this fails WCAG 1.3.1.
Adds loaderLabel prop to provide a visually hidden label for the component.
Deprecates the 'aria-label' prop in favour of the new label prop.

fix #6956
@edleeks87 edleeks87 marked this pull request as ready for review January 27, 2025 11:20
@edleeks87 edleeks87 requested review from a team as code owners January 27, 2025 11:20
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.

Incorrect role given to loader
3 participants