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(RadioGroup): new component #730

Merged
merged 16 commits into from
Oct 27, 2023
Merged

feat(RadioGroup): new component #730

merged 16 commits into from
Oct 27, 2023

Conversation

romhml
Copy link
Collaborator

@romhml romhml commented Sep 23, 2023

πŸ”— Linked issue

Fixes #684

❓ Type of change

Introducing a new component RadioGroup to simplify the use of Radio's in forms and fix accessibility issues.

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

πŸ“š Description

πŸ“ Checklist

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

@vercel
Copy link

vercel bot commented Sep 23, 2023

The latest updates on your projects. Learn more about Vercel for Git β†—οΈŽ

Name Status Preview Updated (UTC)
ui βœ… Ready (Inspect) Visit Preview Oct 27, 2023 1:45pm

@romhml
Copy link
Collaborator Author

romhml commented Sep 28, 2023

@benjamincanac it's ready to go. I'm tempted to merge the Radio and RadioGroup documentation in the docs (does not really make sense to use a Radio on it's own IMO), I kept both for now to make it easier to review.

@matteason
Copy link

Thanks for picking this up @romhml! It's looking great so far

Would it be possible to add a <legend> to the <fieldset> that wraps the radio group? The legend provides the overall label for the group. It's important for accessibility because screen readers will read the legend when focus enters the radio group, which they won't do if, for example, you just label the group with a paragraph above the fieldset

@romhml
Copy link
Collaborator Author

romhml commented Sep 28, 2023

Thanks @matteason! I added a prop / slot to include a legend, wdyt?

@matteason
Copy link

@romhml Perfect, thank you!

@benjamincanac
Copy link
Member

@benjamincanac it's ready to go. I'm tempted to merge the Radio and RadioGroup documentation in the docs (does not really make sense to use a Radio on it's own IMO), I kept both for now to make it easier to review.

It could make sense to merge both indeed. I'm just unsure about the RadioGroup name as it will prevent us from creating a component on top of https://headlessui.com/vue/radio-group later on.

@romhml
Copy link
Collaborator Author

romhml commented Sep 28, 2023

@benjamincanac that's what it's called in every popular libraries I've seen and the MDN Web docs so I'd rather stick with that.

We can always alias the import from headless-ui if we want to use it don't you think? This is already the case for the select menu.

Copy link
Member

Okay let's do this! Do you plan to merge both radio.md and radio-group.md in this PR?

@romhml
Copy link
Collaborator Author

romhml commented Sep 28, 2023

@benjamincanac Done!

@benjamincanac
Copy link
Member

Thanks @romhml, I've made a few changes if you want to take a look!

@benjamincanac
Copy link
Member

I'm merging this, if you see anything that I missed let me know, we'll open a new PR 😊

@benjamincanac benjamincanac merged commit 23d5dc7 into nuxt:dev Oct 27, 2023
2 checks passed
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.

Radios which share a model should have the same name
3 participants