fix(FormControl): allow required check boxes #5027

francinelucca · 2024-09-25T17:16:05Z

Relates to https://github.com/github/primer/issues/3470

FormControl by design does not allow individual Checkbox/Radio to be required. As per:

https://github.com/github/primer/issues/3470#issuecomment-2362018592
fix(FormControl): allow required check boxes #5027 (comment) there may be use cases where an individual Checkbox can be required individually or within a CheckboxGroup (since multiple checkboxes can be checked at once, opposed to radio buttons). This PR introduces functionality to FormControl allow that behavior to happen.

Changelog

New

Adds FormControl tests for required prop used with individual and group checkboxes

Changed

Updates choice group checks in FormControl to determine if the input is a Checkbox and pass down the required prop in which case
Updates CheckboxGroup Playground story to showcase one required Checkbox within a CheckboxGroup
Updates CheckboxGroup story snapshots to account for required change

Rollout strategy

Patch release
Minor release
Major release; if selected, include a written rollout or migration plan
None; if selected, include a brief description as to why

Testing & Reviewing

Verify required checkbox within CheckboxGroup playground story, notice required prop on input in DOM.
There should be no visual changes to FormControl, Checkbox or CheckboxGroup
Verify Default FormControl story now properly marks required attribute on checkbox

Merge checklist

Added/updated tests
Added/updated documentation
Added/updated previews (Storybook)
Changes are SSR compatible
Tested in Chrome
Tested in Firefox
Tested in Safari
Tested in Edge
(GitHub staff only) Integration tests pass at github/github (Learn more about how to run integration tests)

changeset-bot · 2024-09-25T17:16:09Z

🦋 Changeset detected

Latest commit: f7433a2

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

This PR includes changesets to release 1 package

Name	Type
@primer/react	Patch

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

github-actions · 2024-09-25T17:19:10Z

size-limit report 📦

Path	Size
packages/react/dist/browser.esm.js	97.31 KB (-0.31% 🔽)
packages/react/dist/browser.umd.js	97.51 KB (-0.29% 🔽)

…ice-use-required-instead-of-an-asterisk-for-required-fields

broccolinisoup

Thanks for this PR! I left a question.

I also think we should document these accessibility rules about required checkboxes/radio buttons maybe on the form overview page? - I find them a bit complex 😬

@tbenning might help with this I think if you think documenting them is a good idea, too.

Thanks again!!

broccolinisoup · 2024-10-04T02:39:06Z

packages/react/src/__tests__/FormControl.test.tsx

@@ -390,6 +391,41 @@ describe('FormControl', () => {
        expect(consoleSpy).toHaveBeenCalled()
        consoleSpy.mockRestore()
      })
+
+      it('should not add required prop to individual checkbox', async () => {


In other cases when signing up on a site, there may be some options like marketing emails and terms and conditions, where terms and conditions is required but marketing emails is not.

It makes me think what if the terms and conditions option is required (which is in most cases) and there are no other options. Does that mean individual checkboxes can also be required even if there are not a part of group?

Let me know if I am missing something here.

That's a good point, @ichelsea thoughts on this?

I would say yes to that; Indvidual checkboxes can be required if not a part of a group of checkboxes. Are there technical limitations to this?

No technical limitations, just wanted to confirm since that wasn't previously allowed. thanks for the input!

packages/react/src/__tests__/FormControl.test.tsx

github-actions · 2024-10-08T20:22:14Z

👋 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!

primer-integration · 2024-10-08T21:08:55Z

👋 Hi from github/github! Your integration PR is ready: https://github.com/github/github/pull/346259

primer-integration · 2024-10-08T21:29:36Z

🟢 golden-jobs completed with status success.

broccolinisoup · 2024-10-10T05:54:10Z

packages/react/src/FormControl/FormControl.tsx

        // eslint-disable-next-line no-console
-        console.warn('An individual checkbox or radio cannot be a required field.')
+        console.warn('An individual radio cannot be a required field.')


I know console.warn was already here so no need to change - I just wanted to mention that there is a utility function for these cases. And it only runs in the dev environment 🤗

Refactored to use warning function, thanks for pointing that out 🙏

broccolinisoup · 2024-10-10T05:55:51Z

packages/react/src/__tests__/FormControl.test.tsx

+        expect(getByRole('radio')).not.toBeRequired()
+      })
+
+      it('should allow required prop on checkbox if part of CheckboxGroup', async () => {


Should we add a test for "should allow required prop on radio button if part of RadioButtonGroup"?

it's my understanding from this that a radio cannot be required, not even when within a RadioButtonGroup, just the group itself can be required (at the top level). I can add a test for "does not allow required prop on radio button when part of RadioButtonGroup" if you think that'd be worth it

No worries, all good!

broccolinisoup · 2024-10-10T06:39:03Z

packages/react/src/FormControl/FormControl.tsx

@@ -99,9 +100,9 @@ const FormControl = React.forwardRef<HTMLDivElement, FormControlProps>(
        )
      }

-      if (childrenWithoutSlots.find(child => React.isValidElement(child) && child.props?.required)) {
+      if (isRadioInput && childrenWithoutSlots.find(child => React.isValidElement(child) && child.props?.required)) {


Do we also cover the case of "Radio groups can only have the group receive a required setting because you can only select one choice."

Sorry if I missed it!

Yeah so that'd be handled at the RadioGroup level. The FormControl will be contained within the RadioGroup in this case (RadioGroup -> FormControl -> Radio) and doesn't need to be concerned about required being on the parent. Radios will not receive required from the FromControl regardless

Sounds good! Thanks for clarifying 🙌🏻

…ice-use-required-instead-of-an-asterisk-for-required-fields

broccolinisoup

Looks great!! 🔥

Thanks for addressing the feedback and working on this 😊

fix(FormControl): allow required check boxes in CheckboxGroup

2f36653

francinelucca added staff Author is a staff member component: FormControl labels Sep 25, 2024

francinelucca temporarily deployed to github-pages September 25, 2024 17:17 — with GitHub Actions Inactive

github-actions bot temporarily deployed to storybook-preview-5027 September 25, 2024 17:18 Inactive

Create wild-maps-grow.md

86aa75b

francinelucca temporarily deployed to github-pages September 25, 2024 17:26 — with GitHub Actions Inactive

github-actions bot temporarily deployed to storybook-preview-5027 September 25, 2024 17:27 Inactive

francinelucca added the update snapshots label Sep 25, 2024

test(vrt): update snapshots

002f965

github-actions bot removed the update snapshots label Sep 25, 2024

primer bot temporarily deployed to github-pages September 25, 2024 17:47 Inactive

github-actions bot temporarily deployed to storybook-preview-5027 September 25, 2024 17:47 Inactive

Merge branch 'main' into francinelucca/3470-prcformcontrol-best-pract…

fc03b09

…ice-use-required-instead-of-an-asterisk-for-required-fields

francinelucca temporarily deployed to github-pages September 27, 2024 15:37 — with GitHub Actions Inactive

github-actions bot temporarily deployed to storybook-preview-5027 September 27, 2024 15:37 Inactive

Docs(FormControl): use TextInput instead of CheckBox in default story

83595a7

francinelucca temporarily deployed to github-pages October 2, 2024 20:44 — with GitHub Actions Inactive

github-actions bot temporarily deployed to storybook-preview-5027 October 2, 2024 20:44 Inactive

francinelucca marked this pull request as ready for review October 3, 2024 12:45

francinelucca requested review from a team as code owners October 3, 2024 12:45

francinelucca requested review from tbenning and broccolinisoup October 3, 2024 12:45

francinelucca added the status: review needed label Oct 3, 2024

broccolinisoup reviewed Oct 4, 2024

View reviewed changes

francinelucca requested a review from ichelsea October 4, 2024 14:05

fix(FormControl): allow required prop on individual checkboxes

de2566f

github-actions bot added the integration-tests: recommended This change needs to be tested for breaking changes. See https://arc.net/l/quote/tdmpakpm label Oct 8, 2024

francinelucca temporarily deployed to github-pages October 8, 2024 20:23 — with GitHub Actions Inactive

github-actions bot temporarily deployed to storybook-preview-5027 October 8, 2024 20:24 Inactive

francinelucca changed the title ~~fix(FormControl): allow required check boxes in CheckboxGroup~~ fix(FormControl): allow required check boxes Oct 9, 2024

francinelucca requested a review from broccolinisoup October 9, 2024 20:38

broccolinisoup reviewed Oct 10, 2024

View reviewed changes

francinelucca requested a review from broccolinisoup October 10, 2024 14:30

fix(FormControl): use internal warning instead of console.warning

812bc6d

francinelucca temporarily deployed to github-pages October 10, 2024 16:58 — with GitHub Actions Inactive

github-actions bot temporarily deployed to storybook-preview-5027 October 10, 2024 16:59 Inactive

fix(FormControl): rever console.error label change

ccd6082

francinelucca temporarily deployed to github-pages October 10, 2024 17:04 — with GitHub Actions Inactive

github-actions bot temporarily deployed to storybook-preview-5027 October 10, 2024 17:04 Inactive

Merge branch 'main' into francinelucca/3470-prcformcontrol-best-pract…

f7433a2

…ice-use-required-instead-of-an-asterisk-for-required-fields

francinelucca deployed to github-pages October 10, 2024 17:52 — with GitHub Actions View deployment

github-actions bot deployed to storybook-preview-5027 October 10, 2024 17:53 View deployment

broccolinisoup approved these changes Oct 11, 2024

View reviewed changes

francinelucca mentioned this pull request Oct 17, 2024

Docs: add a11y guidance regarding required fields primer/design#878

Merged

francinelucca added this pull request to the merge queue Oct 21, 2024

francinelucca added integration-tests: passing Changes in this PR do NOT cause breaking changes in gh/gh and removed integration-tests: recommended This change needs to be tested for breaking changes. See https://arc.net/l/quote/tdmpakpm labels Oct 21, 2024

Merged via the queue into main with commit 9a30c63 Oct 21, 2024
49 checks passed

francinelucca deleted the francinelucca/3470-prcformcontrol-best-practice-use-required-instead-of-an-asterisk-for-required-fields branch October 21, 2024 19:18

primer bot mentioned this pull request Oct 21, 2024

Release tracking (37.1.0) #5166

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(FormControl): allow required check boxes #5027

fix(FormControl): allow required check boxes #5027

francinelucca commented Sep 25, 2024 •

edited

Loading

changeset-bot bot commented Sep 25, 2024 •

edited

Loading

github-actions bot commented Sep 25, 2024 •

edited

Loading

broccolinisoup left a comment

broccolinisoup Oct 4, 2024

francinelucca Oct 4, 2024

ichelsea Oct 4, 2024

francinelucca Oct 8, 2024

github-actions bot commented Oct 8, 2024

primer-integration bot commented Oct 8, 2024

primer-integration bot commented Oct 8, 2024

broccolinisoup Oct 10, 2024

francinelucca Oct 10, 2024

broccolinisoup Oct 10, 2024

francinelucca Oct 10, 2024

broccolinisoup Oct 11, 2024

broccolinisoup Oct 10, 2024

francinelucca Oct 10, 2024

broccolinisoup Oct 11, 2024

broccolinisoup left a comment

fix(FormControl): allow required check boxes #5027

fix(FormControl): allow required check boxes #5027

Conversation

francinelucca commented Sep 25, 2024 • edited Loading

Changelog

New

Changed

Rollout strategy

Testing & Reviewing

Merge checklist

changeset-bot bot commented Sep 25, 2024 • edited Loading

🦋 Changeset detected

github-actions bot commented Sep 25, 2024 • edited Loading

size-limit report 📦

broccolinisoup left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Oct 8, 2024

primer-integration bot commented Oct 8, 2024

primer-integration bot commented Oct 8, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

broccolinisoup left a comment

Choose a reason for hiding this comment

francinelucca commented Sep 25, 2024 •

edited

Loading

changeset-bot bot commented Sep 25, 2024 •

edited

Loading

github-actions bot commented Sep 25, 2024 •

edited

Loading