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

Add field wrapper around RadioButtons #2098

Closed
wants to merge 2 commits into from

Conversation

connor-baer
Copy link
Member

Addresses #1875.

Purpose

In v6, we wrapped all form fields in a div to make them easier to style. From the migration guide:

Form components are now all wrapped in a div that receives any styles passed through the style or className attributes, including via the Emotion.js css prop. This might break custom styles.

We missed doing this for the RadioButton component.

Approach and changes

  • Wrap the RadioButton component in the FieldWrapper atom and remove the superflous div from the RadioButtonGroup

Definition of done

  • Development completed
  • Reviewers assigned
  • Unit and integration tests
  • Meets minimum browser support
  • Meets accessibility requirements

@connor-baer connor-baer requested a review from a team as a code owner May 11, 2023 09:02
@changeset-bot
Copy link

changeset-bot bot commented May 11, 2023

🦋 Changeset detected

Latest commit: ba74eec

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 Major

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 May 11, 2023

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

Name Status Preview Updated (UTC)
oss-circuit-ui ✅ Ready (Inspect) Visit Preview May 11, 2023 9:02am

@codecov
Copy link

codecov bot commented May 11, 2023

Codecov Report

Merging #2098 (ba74eec) into next (9677a30) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##             next    #2098   +/-   ##
=======================================
  Coverage   96.91%   96.91%           
=======================================
  Files         258      258           
  Lines       23148    23145    -3     
  Branches     2160     2160           
=======================================
- Hits        22434    22432    -2     
  Misses        707      707           
+ Partials        7        6    -1     
Impacted Files Coverage Δ
.../circuit-ui/components/RadioButton/RadioButton.tsx 98.33% <100.00%> (+0.42%) ⬆️
...i/components/RadioButtonGroup/RadioButtonGroup.tsx 97.20% <100.00%> (-0.07%) ⬇️

Copy link
Contributor

@robinmetral robinmetral left a comment

Choose a reason for hiding this comment

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

💯 great! Thanks for aligning this

options.map((option) => (
<RadioButton
{...option}
key={option.label}
Copy link
Contributor

Choose a reason for hiding this comment

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

good idea to use the label as the key!

@connor-baer connor-baer added this to the v7.0 milestone May 17, 2023
@connor-baer connor-baer added the ⛔ do not merge There is a blocker label May 17, 2023
@connor-baer
Copy link
Member Author

Will merge this after merging and rebasing on #2105.

@connor-baer
Copy link
Member Author

Closing in favor of #2124.

@connor-baer connor-baer deleted the feature/radiobutton-wrapper branch August 5, 2023 09:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants