Add Spectrum CheckboxGroup component by devongovett · Pull Request #1003 · adobe/react-spectrum

devongovett · 2020-08-21T20:56:00Z

Followup from #868
Closes #798

This adds a CheckboxGroup component to @react-spectrum/checkbox. It follows the same styling as RadioGroup but for checkboxes. This component accepts Checkbox elements as children, and passes the group state to them via context.

Additional changes to the hooks:

name handling was moved from stately to aria. This was a mistake in RadioGroup since it's web-specific and it's being deprecated and moved to aria in Fix remaining SSR tests #994. Also, for checkbox groups, there's no need to auto generate a form name like for radio groups, because the name isn't used by the browser for behavior. So we only need to pass through the given name for the group to each checkbox.
isDisabled and isReadOnly are now passed through from the group to the checkboxes automatically by storing the values in state.
aria-invalid and required DOM props are removed from individual checkboxes within a group since they don't make sense there. Instead, these are exposed at the group level, e.g. in Spectrum the necessity indicator can be used to specify that the group is required. This is up to individual design systems to implement.
Renamed checkboxGroupProps in the return value to just groupProps to match the pattern of using the aria role as the name of the property.

Also renamed docs to a temp name so they don't get picked up by the deploy to prod before publishing.

adobe-bot · 2020-08-21T21:00:58Z

Build successful! 🎉

adobe-bot · 2020-08-21T21:13:42Z

Build successful! 🎉

adobe-bot · 2020-08-22T00:34:38Z

Build successful! 🎉

adobe-bot · 2020-08-22T05:05:39Z

Build successful! 🎉

Andarist · 2020-08-23T21:09:41Z

+    ...props,
+    isReadOnly: props.isReadOnly || state.isReadOnly,
+    isDisabled: props.isDisabled || state.isDisabled,
+    name: props.name || checkboxGroupNames.get(state)


This doesn't make sense for me unless I miss something. It seems important to keep the same name for all "checkbox group items" as this "groups" them when sending the containing form element.

I was thinking it might be possible to have a single "group" client side, but maybe you submit to a server using separate names for each checkbox.

from what i remember of form submission, there are some specials names that work with some servers? i think the name has to include "[]" on the end
if you don't include that then checkboxes with the same name will overwrite each other
but not everything recognizes that special name syntax, so probably better to have them be individually named and just 'look' like a group

I was thinking it might be possible to have a single "group" client side, but maybe you submit to a server using separate names for each checkbox.

How realistic this use case is? Shouldn't this really be considered bad practice?

Andarist · 2020-08-23T21:11:31Z

 interface CheckboxGroupAria {
  /** Props for the checkbox group wrapper element. */
-  checkboxGroupProps: HTMLAttributes<HTMLElement>,
+  groupProps: HTMLAttributes<HTMLElement>,


while I understand the sentiment to match against the returned role, isn't this confusing (DX-wise)? given that useRadioGroup returns radioGroupProps?

Well, in that case the role is radiogroup.

Right, the role is mainly opaque to users though - the point of React Aria is so people don't have to worry about this. Seems like one might not really know what exact role is being prefilled by React Aria in the returned objects, but they certainly know how the component that they have just used is named.

I don't care about this as much as TS will help me with this anyway but I kinda find the choice somewhat odd as the chosen name focuses on the low-level implementation detail that is hidden from the user, rather than on a high-level feature that is actually used directly by them.

Andarist · 2020-08-23T21:14:14Z

+    },
    addValue(value) {
-      if (props.isReadOnly) {
+      if (props.isReadOnly || props.isDisabled) {


this hasn't been covered by the related unit tests 😉

Andarist · 2020-08-23T21:17:13Z

-    name,
-    value: selectedValue,
+    value: selectedValues,
    setValue,


if addValue, removeValue and toggleValue check props.isReadOnly || props.isDisabled before actually changing the value, shouldn't the same check apply here?

Andarist · 2020-08-23T21:19:59Z

      expect(state.value).toEqual([]);
+      expect(state.isDisabled).toBe(false);
+      expect(state.isReadOnly).toBe(false);
      expect(typeof state.setValue).toBe('function');


this test should also check if isSelected is provided by the hook

Andarist · 2020-08-23T21:20:31Z

+      expect(state.isReadOnly).toBe(true);

      return null;
    }


would be great to add a simple test for isSelected functionality as well

I guess. We tend not to do too much unit testing of the stately part by itself given that it's already covered by the ARIA/Spectrum tests indirectly. This tends to lead to more maintainable tests. But I suppose since the others are already tested I can add a quick one.

Andarist · 2020-08-24T07:11:08Z

@@ -160,7 +159,7 @@ describe('useCheckboxGroup', () => {
  });

  it('sets aria-disabled when isDisabled is true', () => {


I believe that those test names should be adjusted now as a little more than just aria-disabled is being tested here

adobe-bot · 2020-08-25T01:14:08Z

Build successful! 🎉

Co-authored-by: Danni <drobinson@livefyre.com>

adobe-bot · 2020-08-25T01:47:24Z

Build successful! 🎉

Add Spectrum CheckboxGroup component

4e97d4b

devongovett requested review from dannify and snowystinger as code owners August 21, 2020 20:56

Rename checkboxGroupProps to groupProps to match ARIA role

4d9157f

devongovett added 2 commits August 21, 2020 17:14

Improve handling of individual checkbox props within a group

de522a2

lint

864bc6e

devongovett added 3 commits August 21, 2020 19:29

Add docs for Spectrum CheckboxGroup

97ce54c

Hide docs from prod build that are not yet released

e057fa9

Add docs for useCheckboxGroup

1c8a943

Andarist reviewed Aug 23, 2020

View reviewed changes

Andarist reviewed Aug 24, 2020

View reviewed changes

review updates

7d2829a

dannify requested changes Aug 25, 2020

View reviewed changes

Comment thread packages/@react-aria/checkbox/docs/useCheckboxGroup.mdx Outdated

dannify requested changes Aug 25, 2020

View reviewed changes

Comment thread packages/@react-spectrum/checkbox/docs/CheckboxGroup.mdx Outdated

Apply suggestions from code review

8ad361e

Co-authored-by: Danni <drobinson@livefyre.com>

dannify approved these changes Aug 25, 2020

View reviewed changes

dannify merged commit de800ea into main Aug 25, 2020

dannify deleted the checkbox-group branch August 25, 2020 01:54

		@@ -160,7 +159,7 @@ describe('useCheckboxGroup', () => {
		});

		it('sets aria-disabled when isDisabled is true', () => {

Conversation

devongovett commented Aug 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adobe-bot commented Aug 21, 2020

Uh oh!

adobe-bot commented Aug 21, 2020

Uh oh!

adobe-bot commented Aug 22, 2020

Uh oh!

adobe-bot commented Aug 22, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adobe-bot commented Aug 25, 2020

Uh oh!

Uh oh!

Uh oh!

adobe-bot commented Aug 25, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

devongovett commented Aug 21, 2020 •

edited

Loading