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

Allow passing custom aria-describedby values to form components #1813

Merged
merged 3 commits into from
Oct 26, 2022

Conversation

robinmetral
Copy link
Contributor

@robinmetral robinmetral commented Oct 25, 2022

Follows up on #1795

Purpose

#1795 programatically associated input validationHints with input elements using aria-describedby.

However, it overlooked the case where an implementation wants to pass multiple descriptions to aria-describedby. In the current implementation, any custom id passed to the attribute would be overridden by the generated id of the validationHint element.

We're already running into this use case in Circuit UI itself, specifically in the CurrencyInput component, where the input needs to be described both by its validationHint but also by the currency suffix (otherwise the currency is left out of the accessibility properties of the input.

Approach and changes

Combine the validationHintId (generated internally) with any custom aria-describedby value (a list of ids, separated with spaces) passed to the component.

Covered extensively with the awesome @testing-library matchers 💙

Definition of done

  • Development completed
  • Reviewers assigned
  • Unit and integration tests 👉 extended the "Accessibility > Labeling" section of the specs
  • Meets minimum browser support
  • Meets accessibility requirements

@changeset-bot
Copy link

changeset-bot bot commented Oct 25, 2022

🦋 Changeset detected

Latest commit: e619bee

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 Patch

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 Oct 25, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
oss-circuit-ui ✅ Ready (Inspect) Visit Preview Oct 26, 2022 at 9:48AM (UTC)

@robinmetral robinmetral changed the title Allow passing custom aria-describedby values to form component Allow passing custom aria-describedby values to form components Oct 25, 2022
@codecov
Copy link

codecov bot commented Oct 25, 2022

Codecov Report

Merging #1813 (e619bee) into next (bd184b5) will decrease coverage by 0.03%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             next    #1813      +/-   ##
==========================================
- Coverage   91.85%   91.81%   -0.04%     
==========================================
  Files         172      172              
  Lines        3534     3544      +10     
  Branches     1134     1139       +5     
==========================================
+ Hits         3246     3254       +8     
- Misses        268      270       +2     
  Partials       20       20              
Impacted Files Coverage Δ
...ckages/circuit-ui/components/Checkbox/Checkbox.tsx 100.00% <100.00%> (ø)
...es/circuit-ui/components/ImageInput/ImageInput.tsx 83.49% <100.00%> (+0.32%) ⬆️
packages/circuit-ui/components/Input/Input.tsx 93.75% <100.00%> (+0.27%) ⬆️
...i/components/RadioButtonGroup/RadioButtonGroup.tsx 88.88% <100.00%> (+1.38%) ⬆️
packages/circuit-ui/components/Select/Select.tsx 95.12% <100.00%> (+0.25%) ⬆️
...mponents/NotificationBanner/NotificationBanner.tsx 83.60% <0.00%> (-3.28%) ⬇️

Robin Métral added 2 commits October 26, 2022 11:42
…mponents

Namely ImageInput, Input, RadioButtonGroup and Select
@sumup-oss sumup-oss deleted a comment from sumup-clark bot Oct 26, 2022
@robinmetral robinmetral marked this pull request as ready for review October 26, 2022 09:45
@robinmetral robinmetral requested a review from a team as a code owner October 26, 2022 09:45
@robinmetral robinmetral added 🐞 bug Something isn't working as it should ♿ accessibility Improves usability ⚛️ component Changes to a React component labels Oct 26, 2022
Comment on lines +193 to +195
const descriptionIds = `${
descriptionId ? `${descriptionId} ` : ''
}${validationHintId}`;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Couldn't think of anything cleaner here. But in the end I think it's pretty clear, and tests also document the reasoning behind this. Feedback welcome.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
♿ accessibility Improves usability 🐞 bug Something isn't working as it should 🗂 circuit-ui ⚛️ component Changes to a React component
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant