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

radio options #107

Merged
merged 24 commits into from
Dec 1, 2022
Merged

radio options #107

merged 24 commits into from
Dec 1, 2022

Conversation

diogob
Copy link
Contributor

@diogob diogob commented Nov 14, 2022

This PR will add three properties to the Form component.

  • radio takes a list of fields to be rendered as radio button groups.
  • radioGroupWrapperComponent takes a tag name or a component which will wrap radio button groups.
  • radioWrapperComponent takes a tag name or a component which will wrap individual radio buttons.
  • radioComponent takes a tag name or a component which will render each radio button.

They are all optional therefore no breaking change is introduced.

This should address #54

TODO

  • Rename radioWrapperComponent to radioGroupComponent.
  • Add new radioWrapperComponent to wrap around individual radio options.
  • Ensure default tags have all the proper accessibility features.

@netlify
Copy link

netlify bot commented Nov 14, 2022

Deploy Preview for remix-forms canceled.

Name Link
🔨 Latest commit b33b82b
🔍 Latest deploy log https://app.netlify.com/sites/remix-forms/deploys/6388e11304d2010009fc9b40

@diogob diogob force-pushed the radio-options branch 9 times, most recently from 2f0080d to 9d6434d Compare November 16, 2022 15:38
@diogob diogob marked this pull request as ready for review November 17, 2022 21:34
apps/web/app/routes/examples/forms/radio-buttons.tsx Outdated Show resolved Hide resolved
apps/web/app/routes/examples/forms/radio-buttons.tsx Outdated Show resolved Hide resolved
apps/web/app/routes/examples/forms/radio-buttons.tsx Outdated Show resolved Hide resolved
apps/web/app/routes/examples/forms/radio-buttons.tsx Outdated Show resolved Hide resolved
apps/web/app/ui/radio.tsx Show resolved Hide resolved
@diogob diogob force-pushed the radio-options branch 2 times, most recently from d07c32b to 562d482 Compare November 23, 2022 14:53
@diogob
Copy link
Contributor Author

diogob commented Nov 23, 2022

@danielweinmann I think we should default the radio group wrapper to a fieldset and the group label to a tag legend to follow the mdn example (the second one).

Thoughts?

@danielweinmann
Copy link
Contributor

@danielweinmann I think we should default the radio group wrapper to a fieldset and the group label to a tag legend to follow the mdn example (the second one).

Thoughts?

👌🏼

@danielweinmann
Copy link
Contributor

@diogob, I think radioGroupWrapperComponent could be called simply radioGroupComponent. What do you say?

@diogob diogob force-pushed the radio-options branch 2 times, most recently from 2593fd3 to fcb6bc8 Compare December 1, 2022 02:28
@diogob
Copy link
Contributor Author

diogob commented Dec 1, 2022

@danielweinmann you can take a look now, introducing unit tests will require a little more research and I'll do it in a future PR.

* Apply code review suggestions

Co-authored-by: Daniel Weinmann <danielweinmann@gmail.com>
@danielweinmann danielweinmann merged commit 1c4fa3b into main Dec 1, 2022
@danielweinmann danielweinmann deleted the radio-options branch December 1, 2022 18:16
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.

2 participants