-
Notifications
You must be signed in to change notification settings - Fork 537
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
Updates label and legend styles for Checkbox, Radio, CheckboxGroup, and RadioGroup #5333
Conversation
🦋 Changeset detectedLatest commit: 2a87961 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
Uh oh! @mperrotti, the image you shared is missing helpful alt text. Check your pull request body. Alt text is an invisible description that helps screen readers describe images to blind or low-vision users. If you are using markdown to display images, add your alt text inside the brackets of the markdown image. Learn more about alt text at Basic writing and formatting syntax: images on GitHub Docs.
|
👋 Hi, this pull request contains changes to the source code that github/github depends on. If you are GitHub staff, we recommend testing these changes with github/github using the integration workflow. Thanks! |
size-limit report 📦
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM code wise, just a few questions.
Also since this has the potential to touch a lot of areas indirectly, I'd recommend running the integration tests and doing some manual testing in RL to make sure everything still looks good
@@ -172,17 +172,15 @@ const FormControl = React.forwardRef<HTMLDivElement, FormControlProps>( | |||
{slots.leadingVisual} | |||
</Box> | |||
)} | |||
{!slots.label?.props.visuallyHidden || slots.caption ? ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there a reason we don't need this check anymore?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I couldn't figure out why needed this check, and removing it doesn't break any VRTs or unit tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did some local testing and couldn't force a difference either. Went down a bit of a rabbit hole and saw you authored this change so I'll trust you, approving! Let me know if you want help running integration checks or testing in RL
@@ -54,7 +54,6 @@ const InputLabel: React.FC<React.PropsWithChildren<Props>> = ({ | |||
display: 'block', | |||
color: disabled ? 'fg.muted' : 'fg.default', | |||
cursor: disabled ? 'not-allowed' : 'pointer', | |||
alignSelf: 'flex-start', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just making sure this is an intended change? as it doesn't seem directly correlated with Checkbox
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh I mis-read alignSelf
as alignItems
and thought this rule wouldn't have any effect.
I'm not certain what it does, but I'll add it back just in case it's needed for something 😅
…com:primer/react into mp/update-checkbox+radio-group-label-styles
Closes #5311
Screenshots
Before
After
Changelog
New
Changed
<legend>
bold and default font size<label>
default font weightRemoved
Rollout strategy
Testing & Reviewing
Merge checklist