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 a CheckboxGroup component #1815

Closed
robinmetral opened this issue Oct 26, 2022 · 13 comments
Closed

Add a CheckboxGroup component #1815

robinmetral opened this issue Oct 26, 2022 · 13 comments
Assignees
Labels
🗂 circuit-ui ⚛️ component Changes to a React component feature A new feature or enhancement
Milestone

Comments

@robinmetral
Copy link
Contributor

robinmetral commented Oct 26, 2022

Visual

No visuals yet. Refer to the RadioButtonGroup for inspiration

Context

Circuit UI has a Checkbox component but no CheckboxGroup component.

Such a component is used in situations when the checkboxes are all grouped under one label, for example (from MDN):

Choose your monster's features:

  • Scales
  • Horns

This example shouldn't just use two checkboxes—it requires extra markup to make sure checkboxes are programatically grouped:

<fieldset>
    <legend>Choose your monster's features:</legend>
    <label for="scales">Scales</label>
    <input type="checkbox" id="scales" name="scales" checked>
    <label for="horns">Horns</label>
    <input type="checkbox" id="horns" name="horns">
</fieldset>

A similar markup is used by the RadioButtonGoup component (replacing checkboxes by radio buttons). The SelectorGroup component (when passed the multiple prop) has the same underlying markup, but a different UI.

@robinmetral robinmetral added feature A new feature or enhancement ⚛️ component Changes to a React component 🗂 circuit-ui labels Oct 26, 2022
@long-lazuli
Copy link
Contributor

long-lazuli commented Oct 26, 2022

Hi,
Just saw this Issue,
We might have something relatively close to what you are proposing.
image
image

Would that be useful to make a PR with our implementation ?
Something we have also, is a selectAll option.

How do you imagine the surface API ?

We have something close to :

  <Label id="monsterAttributesLabel">Choose your monster's features:</Label>
  <CheckboxesGroup
    data-testid={`monster-attributes-container`}
    name={`monster-attributes`}
    aria-labelledby={`monsterAttributesLabel`}
    toggleAllLabel="toggle all attributes!"
    onChange={(e) => setSelectedCheckboxes(e.details.value)}
    required={true}
    readOnly={false}
    disabled={false}
    minRequired={1}
    options={[
      { label: 'Scales', value: 'scales', checked: true },
      { label: 'Horns', value: 'horns', checked: false },
      { label: 'Claws', value: 'claws', disabled: true },
    ]}
  />

Many of the properties above are optional. I just put many things to show you quickly what we have.

Because of some inconsistency in style, for <fieldset>, we use a <div[role='group']> as a container for the checkboxes.

We also not have made the Label inside this component, but we could easily change that.

The onChange pass CustomEvent<{value: string[]}>.

the selectAll kind of checkbox has no name and is not sended through the form. therefore, it's checked status is not important. but we might add an aria-checked='mixed' when in the indeterminate state, for Safari to read that correctly. (test has to be done, this can be related to this too old PR.)

You can also omit the onChange property, and leave the form handle the values; as it is just <input[type='checkbox']> as you would expect.
In this case, we can probably allow passing defaultValue to the options, and not pass the onChange on the component.

@robinmetral
Copy link
Contributor Author

Thanks for this @long-lazuli! As a first step I'd focus on a simple two-state checkbox group (which doesn't require design changes, just improves DX and accessibility).

What you're describing is the tri-state checkbox pattern and I think we could consider it as a next step, depending on demand from teams. We'd start with a design phase for this as well

@tranhoangan22 tranhoangan22 self-assigned this Nov 14, 2022
@long-lazuli
Copy link
Contributor

It's in fact a group of two-state checkboxes,
the undefined-state checkbox you see is optional, and serve just to uncheck/check all the other checkboxes...

@robinmetral
Copy link
Contributor Author

Thanks for clarifying 🙌 I'd still start with the two-state group (technical improvement, no design changes) and then look into more complex patterns as a second step 🙂 The one you're describing might be tricky in terms of a11y—at least I can't find examples of it and we'd need to do more research (pointers welcome)

@long-lazuli
Copy link
Contributor

long-lazuli commented Nov 17, 2022

I know we need to investigate that ( see #1415 ).
Would you like a PR without the 'checkAll/uncheckAll' for a start ?

Recently, we actually copied this component from one project to another one; but it would be better if it was here in Circuit.

@robinmetral
Copy link
Contributor Author

I think that @tranhoangan22 wanted to contribute the CheckboxGroup (two state) after the v6 stable is released 🙂 and after that I'll also hopefully have a bit more time to look into the tri-state pattern. At that point I'll also involve designers: any UI changes (not needed for the two-state CheckboxGroup) should start off in Figma specs

@long-lazuli
Copy link
Contributor

long-lazuli commented Nov 17, 2022

Wouldn't that be simpler to just rename RadioButtonGroup to ClickableGroup and just add a multiple props ?

Then for options, we do something like :

          options.map(
            ({ label: optionLabel, value, className, style, ...rest }) => (
              <div
                key={value && value.toString()}
                className={className}
                style={style}
              >
                {createElement(multiple ? Checkbox : RadioButton, {
                  ...{ ...rest, value, name, required, onChange },
                  checked: value === activeValue,
                  label: optionLabel,
                })}
              </div>
            ),
          )}

It would force us to take care of the homogeneity of the two types (RadioButtonProps & CheckboxProps) as well...
What do you think ?

@robinmetral
Copy link
Contributor Author

👀 TBH that's a pretty good idea. The components would probably be fairly similar. I'd probably still export a RadioButtonGroup and a CheckboxGroup for clarity and backwards compatibility, but for example the CheckboxGroup could just be a <RadioButtonGroup multiple />.

Still to be explored to make sure that we can extend the right HTMLAttributes but I like the idea 💡 we also already do something like this in the SelectorGroup, too.

@tranhoangan22
Copy link
Contributor

tranhoangan22 commented Nov 18, 2022

I've noticed there are some differences between the RadioButton and Checkbox:

  • There is a validationHint prop on a Checkbox while there is none on the RadioButton.
  • The label for a Checkbox is defined by the Checkbox element's children while the label for a RadioButton is defined by the label prop.
  • A CheckboxGroup would probably have a default state where more than one Checkbox is checked, which is not possible for a RadioButtonGroup.

I would think it may be still a better idea to keep the implementations of RadioButtonGroup and CheckboxGroup separate to ensure the clarity and independence between these two components.

@robinmetral
Copy link
Contributor Author

Sounds good to me. Just FYI:

  • There is a validationHint prop on a Checkbox while there is none on the RadioButton.

In the CheckboxGroup, individual checkboxes should (probably) not allow a validationHint (TBC)

  • The label for a Checkbox is defined by the Checkbox element's children while the label for a RadioButton is defined by the label prop.

This should be aligned in a future major. All input components should accept a label prop, not children. But out of scope of this one.

Let's start with separate implementations for now, they can always be merged moving forward when form components are more consistent

@github-actions
Copy link
Contributor

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Add a comment explaining why the issue is still relevant to prevent it from being closed.

@github-actions github-actions bot added the stale label May 11, 2023
@connor-baer
Copy link
Member

This is being worked on in #1851.

@connor-baer connor-baer removed the stale label May 11, 2023
@connor-baer connor-baer added this to the v6.x milestone May 22, 2023
@connor-baer
Copy link
Member

Fantastic news! The CheckboxGroup was released as part of Circuit UI v6.9 🎉

Big thanks to @tranhoangan22 for the contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🗂 circuit-ui ⚛️ component Changes to a React component feature A new feature or enhancement
Projects
None yet
Development

No branches or pull requests

4 participants