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 CheckboxGroup and RadioGroup components #830

Open
wants to merge 25 commits into
base: main
Choose a base branch
from

Conversation

joshfarrant
Copy link
Contributor

@joshfarrant joshfarrant commented Nov 25, 2024

Summary

Adds CheckboxGroup and RadioGroup components which, for the time being, are just implementations of the new private ControlGroup component. These components allow us to group Radio and Checkbox components using a <fieldset> and label them with a <legend>, providing a more accessible and semantic way to group related form elements.

List of notable changes:

  • Adds ControlGroup component (alternate name suggestions welcome)
  • Adds CheckboxGroup and RadioGroup components, tests, and stories

Still to do (in future PRs)

  • Add documentation for the new components
  • Consume new components in our existing documentation and stories

What should reviewers focus on?

  • There were some spacing inconsistencies between the Figma file I was using for reference and our actual form element sizes in Primer Brand.
    • Specifically, font sizes varied between the two. Figma has the font sizes of the Radio and Checkbox labels, as well as the validation, at 14px, but in PB they're all 16px. Could you advise which way to go with this please @danielguillan, as well as giving the design of the components a quick check too please?
  • I used the new(ish) CSF3 in the stories. The only difference is the fact that stories now export objects with a render function, rather than exporting components. Although I didn't use it here, this has the added benefit of letting you create/update component stories from within the Storybook UI and save the new stories to disk. Happy to revert this if we'd prefer to keep things consistent, but at some point it might be a good idea to update all stories to CSF3 as one less thing to worry about for future Storybook upgrades.

Steps to test:

  1. Take a look at the Storybook

Supporting resources (related issues, external links, etc):

Contributor checklist:

  • All new and existing CI checks pass
  • Tests prove that the feature works and covers both happy and unhappy paths
  • Any drop in coverage, breaking changes or regressions have been documented above
  • UI Changes contain new visual snapshots (generated by adding update snapshots label to the PR)
  • All developer debugging and non-functional logging has been removed
  • Related issues have been referenced in the PR description

Reviewer checklist:

  • Check that pull request and proposed changes adhere to our contribution guidelines and code of conduct
  • Check that tests prove the feature works and covers both happy and unhappy paths
  • Check that there aren't other open Pull Requests for the same update/change

Screenshots:

Please try to provide before and after screenshots or videos

RadioGroup CheckboxGroup

image

image

Copy link

changeset-bot bot commented Nov 25, 2024

🦋 Changeset detected

Latest commit: 8e91013

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 6 packages
Name Type
@primer/react-brand Minor
@primer/brand-primitives Minor
@primer/brand-e2e Minor
@primer/brand-fonts Minor
@primer/brand-config Minor
@primer/brand-storybook Minor

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

Copy link
Contributor

github-actions bot commented Nov 25, 2024

🟢 No design token changes found

Choose a reason for hiding this comment

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

Copilot reviewed 16 out of 26 changed files in this pull request and generated no suggestions.

Files not reviewed (10)
  • packages/react/src/forms/InputGroup/InputGroup.module.css: Language not supported
  • packages/react/src/forms/RadioGroup/RadioGroup.tsx: Evaluated as low risk
  • packages/react/src/forms/CheckboxGroup/CheckboxGroup.stories.tsx: Evaluated as low risk
  • packages/react/src/forms/CheckboxGroup/CheckboxGroup.test.tsx: Evaluated as low risk
  • packages/react/src/forms/index.ts: Evaluated as low risk
  • packages/react/src/forms/CheckboxGroup/index.ts: Evaluated as low risk
  • .changeset/popular-seas-yawn.md: Evaluated as low risk
  • packages/react/src/forms/InputGroup/index.ts: Evaluated as low risk
  • packages/react/src/forms/RadioGroup/RadioGroup.stories.tsx: Evaluated as low risk
  • packages/react/src/forms/RadioGroup/index.ts: Evaluated as low risk
Comments skipped due to low confidence (3)

packages/react/src/forms/CheckboxGroup/CheckboxGroup.visual.spec.ts:8

  • Consider removing the eslint-disable comment and ensuring that the code adheres to the i18n-text/no-en rule.
// eslint-disable-next-line i18n-text/no-en

packages/react/src/forms/InputGroup/InputGroup.tsx:20

  • The error message should be more concise and specific, e.g., 'useInputGroup must be used within an InputGroupProvider.'
throw new Error('useInputGroup must be used within an InputGroupProvider. Did you forget to wrap your component in a <InputGroupProvider>?')

packages/react/src/forms/CheckboxGroup/CheckboxGroup.tsx:14

  • [nitpick] Consider adding a comment explaining that CheckboxGroup is currently just an alias for InputGroup to provide context for future maintainers.
export const CheckboxGroup = InputGroup

Tip: Copilot only keeps its highest confidence comments to reduce noise and keep you focused. Learn more

Copy link
Contributor

github-actions bot commented Nov 25, 2024

⚠️ Visual differences found

Our visual comparison tests found UI differences.

Please review the differences by using the test artifacts to ensure that the changes were intentional.

Artifacts can be downloaded and reviewed locally.

Download links are available at the bottom of the workflow summary screen.

Example:

artifacts section of workflow run

If the changes are expected, please run npm run test:visual:update-snapshots to replace the previous fixtures.

Review visual differences


.ControlGroup__validation-icon--success {
position: relative;
top: -1.6px;
Copy link
Collaborator

@rezrah rezrah Nov 27, 2024

Choose a reason for hiding this comment

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

Just curious why this is -1.6px vs -1px? I think -1px helps the check mark be more vertically aligned to the center of the first adjacent character, but -1.6px is so specific I'm probably missing something:

With -1px:

Screenshot 2024-11-27 at 13 47 43

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Again, pulled from FormControl to keep things consistent

https://github.com/primer/brand/blob/main/packages/react/src/forms/FormControl/FormControl.module.css#L94-L97

Happy to modify it if you'd prefer though?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, we've had this conversation before, just in the other direction 😄

It was 1px, but after a brief conversation (here) we settled on 1.6px.

4f50c91#diff-66f88db62ba93924cf6de6ae216861c98149f0122e02c93173b5778c43cd4b57R95-R98

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah okay 🤣 .. let's keep it the same then.. i used a visual guide in the other screenshot so that's probably more correct. Although could this be a bit different because the text is 100 vs 200?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Although could this be a bit different because the text is 100 vs 200?

Yea I just realised the same thing, I think that accounts for the difference. I've adjusted the values visually to account for the smaller text size, but if you want to have another look and possibly tweak them then just let me know and I'll update the values in the PR

},
}

export const Inline: Story = {
Copy link
Collaborator

@rezrah rezrah Nov 27, 2024

Choose a reason for hiding this comment

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

Could we move this story into a separate CheckboxGroup.features.stories.tsx file and organize it inside a CheckboxGroup/Examples or CheckboxGroup/Features subfolder please? Probably former in this case as it's not a feature of the component.

I know the other form control stories don't follow this format. It's because those predate the Primer ADR around story organization and also need to be updated at some point.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let me know what you think about my approach here

17aefc5

I wanted to keep the interactivity of the Inline story as it currently works nicely with the controls, but I didn't want to duplicate the controls set up (args, argTypes) across both files, so I've brought the meta object defined in the main story file into the examples file and reused it there, just with a modified title.

lmk if you're happy with this


export default meta

type Story = StoryObj<MetaProps>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does Storybook provide a built-in type for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

StoryObj is the built-in, recommended type. See the example in the docs here.

<Radio name="demo" value="three" />
</FormControl>

<RadioGroup.Validation variant="success">Great job!</RadioGroup.Validation>
Copy link
Collaborator

Choose a reason for hiding this comment

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

There's a Jest warning related to validation prop in the test logs. Possible to take a look at fixing that please?

https://github.com/primer/brand/actions/runs/12053705065/job/33609998572?pr=830#step:10:53

Screenshot 2024-11-27 at 16 59 11

(Doesn't seem to be happening for CheckboxGroup FWIW)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yea, thanks for raising this. I spotted this the other day and I meant to dig into it more.

It boils down to the fact that, in FormControl.tsx, we forward validationStatus to the Radio component, but Radio doesn't implement it, so it just ends up in the HTML.

I'm not sure if I can just stop FormControl from forwarding validationStatus to Radio, or whether Radio should actually be using it to set aria-invalid as Checkbox does.

My inclination is to tackle this in its own PR as it's not an issue introduced in this PR, wdyt?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thinking about it though, I'm not entirely sure how an individual Radio control (not a group, just a single control) could ever be invalid 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've removed the prop forwarding and resolved the console.error in the tests. Removing the forwarding hasn't modified the behaviour of FormControl or Radio in any way as the forwarded prop was never used.

8e91013


The `RadioGroup` family of components provide a way to group related [Radio](/components/Radio) components together with supporting content such as labels, captions, and validation messages. It creates a semantic fieldset structure that helps maintain proper form hierarchy and accessibility.

> **Important**: When using Radio components within a RadioGroup, ensure you provide the same `name` prop to each Radio. This ensures they are correctly grouped and only one option can be selected at a time.
Copy link
Collaborator

@rezrah rezrah Nov 27, 2024

Choose a reason for hiding this comment

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

Suggested change
> **Important**: When using Radio components within a RadioGroup, ensure you provide the same `name` prop to each Radio. This ensures they are correctly grouped and only one option can be selected at a time.
> **Important**: When using Radio components within a radio group, ensure you provide the same `name` prop to each Radio. This ensures they are correctly grouped and only one option can be selected at a time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've just replaced this file with a (near) copy of PRC, lmk what you think

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants