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

CED-1815 Fix accessibility issue in v3 EsRadioButton #1506

Merged
merged 18 commits into from
Sep 5, 2024

Conversation

tomleo
Copy link
Member

@tomleo tomleo commented Sep 3, 2024

πŸ”— Linked issue

❓ Type of change

  • πŸ“– Documentation (updates to the documentation or readme)
  • 🐞 Bug fix (a non-breaking change that fixes an issue)
  • πŸ‘Œ Enhancement (improving an existing functionality like performance)
  • ✨ New feature (a non-breaking change that adds functionality)
  • ⚠️ Breaking change (fix or feature that would cause existing functionality to change)

πŸ“š Description

  • Renamed EsRadioButton to EsRadioInput as there's no buttons involved with this component. The odd naming is an artifact of the radio component it's wrapping PrimeVue RadioButton
  • Added a EsRadioInputGroup component to match how we've been using b-form-radio-group see JIRA for comtext
  • Wrapped all EsRadioInput examples in EsRadioInputGroup components to ensure fieldset & legend are present (for a11y reasons)

πŸ₯Ό Testing

  • Manual

🧐 Feedback Requested / Focus Areas

  • When going through implementation usage across downstream repos, we make heavy use of b-form-group as well. I started to implement a similar component, but that seemed out-of-scope for this. Something we may want to implement down the line though.
  • I removed the area-labledby prop present in the bootstrap-vue's radio-input-group, based on a11y documentation https://www.w3.org/WAI/tutorials/forms/grouping/#radio-buttons I think the fieldset should be the proper way to handle this.

πŸ“ Checklist

  • I have linked an issue or discussion.
  • I have updated the documentation accordingly.
  • I have documented testing approach

@tomleo tomleo changed the base branch from main to esds-3.0-vue3-primevue September 3, 2024 20:29
Copy link
Collaborator

@ericdouglaspratt ericdouglaspratt left a comment

Choose a reason for hiding this comment

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

Radio button is a pretty standard name across the entire industry (Wikipedia) that has a specific etymology. It's also called that in the ESDS 2.0 Figma.

I'd recommend keeping the name, at least on the docs site and in the UI.

@tomleo
Copy link
Member Author

tomleo commented Sep 4, 2024

Radio button is a pretty standard name across the entire industry (Wikipedia) that has a specific etymology. It's also called that in the ESDS 2.0 Figma.

I'd recommend keeping the name, at least on the docs site and in the UI.

Oh weird, bootstrap seems to follow the actual markup input type="radio" with their naming convention of b-form-radio, but aligning with Figma is probably most important. I'll revert back.

Copy link
Contributor

@hroth1994 hroth1994 left a comment

Choose a reason for hiding this comment

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

A few comments on the documentation

es-ds-docs/pages/molecules/radio-button.vue Outdated Show resolved Hide resolved
es-ds-docs/pages/molecules/radio-button.vue Outdated Show resolved Hide resolved
es-ds-docs/pages/molecules/radio-button.vue Outdated Show resolved Hide resolved
es-ds-docs/pages/molecules/radio-button.vue Outdated Show resolved Hide resolved
es-ds-docs/pages/molecules/radio-button.vue Outdated Show resolved Hide resolved
Copy link
Contributor

@hroth1994 hroth1994 left a comment

Choose a reason for hiding this comment

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

Seeing that the linting check is failing (maybe eslint and prettier are fighting?) but otherwise LGTM! 🌟

@tomleo tomleo merged commit 49b7228 into esds-3.0-vue3-primevue Sep 5, 2024
1 check passed
@tomleo tomleo deleted the CED-1815 branch September 5, 2024 17:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants