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

Align API and styles of boolean inputs #2105

Merged
merged 12 commits into from
May 22, 2023

Conversation

connor-baer
Copy link
Member

@connor-baer connor-baer commented May 12, 2023

Addresses #1875.

Purpose

The Checkbox, RadioButton, and Selector components and their grouped variants provide a way for users to select one or multiple options from a predefined list. Conidering their similarities, it's surprising how inconsistent their user experience and APIs are.

This PR aims to align the components as much as possible without introducing breaking changes.

Approach and changes

  • Consistency: Use the internal atomic field component in the CheckboxGroup and SelectorGroup components. Align the types for the options prop. Added the ability to mark fields as optional using the new required and optionalLabel props.
  • State: Added support for components to be used as uncontrolled components for better compatibility with form libraries such as react-hook-form. Group props take precedence over option props. New props: defaultValue, onBlur.
  • Deprecations: Deprecated the RadioButton and Selector components with plans to remove them in v7. They only make sense in groups and using the prebuilt RadioButtonGroup and SelectorGroup components ensures accessibility. Kept the Checkbox component which can be used standalone e.g. for terms & conditions.
  • Documentation: Added more detailed usage instructions. Used more realistic example content in the stories. Grouped related stories to shorten the length of the docs page and reduce our usage of Chromatic snapshots.
  • Tests: Removed all snapshot tests since styles are tested by the visual story tests powered by Chromatic. Aligned unit tests to be consistent between all components.

Definition of done

  • Development completed
  • Reviewers assigned
  • Unit and integration tests
  • Meets minimum browser support
  • Meets accessibility requirements

@changeset-bot
Copy link

changeset-bot bot commented May 12, 2023

🦋 Changeset detected

Latest commit: 5c0b401

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

This PR includes changesets to release 1 package
Name Type
@sumup/circuit-ui 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

@vercel
Copy link

vercel bot commented May 12, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
oss-circuit-ui ✅ Ready (Inspect) Visit Preview 1 resolved May 22, 2023 3:49pm

@codecov
Copy link

codecov bot commented May 12, 2023

Codecov Report

Merging #2105 (5c0b401) into main (b9500fc) will increase coverage by 0.05%.
The diff coverage is 97.50%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2105      +/-   ##
==========================================
+ Coverage   92.04%   92.10%   +0.05%     
==========================================
  Files         170      170              
  Lines        3571     3595      +24     
  Branches     1248     1266      +18     
==========================================
+ Hits         3287     3311      +24     
  Misses        263      263              
  Partials       21       21              
Impacted Files Coverage Δ
...cuit-ui/components/FieldAtoms/FieldDescription.tsx 100.00% <ø> (ø)
...ircuit-ui/components/FieldAtoms/FieldLabelText.tsx 90.00% <ø> (ø)
...t-ui/components/FieldAtoms/FieldValidationHint.tsx 94.59% <ø> (ø)
...cuit-ui/components/SelectorGroup/SelectorGroup.tsx 86.66% <92.30%> (+2.66%) ⬆️
...cuit-ui/components/CheckboxGroup/CheckboxGroup.tsx 90.90% <100.00%> (+2.67%) ⬆️
...s/circuit-ui/components/FieldAtoms/FieldLabels.tsx 100.00% <100.00%> (ø)
...circuit-ui/components/FieldAtoms/FieldWrappers.tsx 100.00% <100.00%> (ø)
.../circuit-ui/components/RadioButton/RadioButton.tsx 92.30% <100.00%> (+1.39%) ⬆️
...i/components/RadioButtonGroup/RadioButtonGroup.tsx 90.90% <100.00%> (+2.02%) ⬆️
...ckages/circuit-ui/components/Selector/Selector.tsx 90.24% <100.00%> (+1.05%) ⬆️

@@ -29,8 +29,7 @@ const baseStyles = () => css`
display: block;
color: var(--cui-fg-subtle);

// TODO: Remove the next line once the Selector component is wrapped in the FieldWrapper component.
Copy link
Member Author

Choose a reason for hiding this comment

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

This component is no longer used in the Selector component.

@@ -186,7 +187,7 @@ const RadioButtonInput = styled('input')<InputElProps>(
);

/**
* RadioButton component for forms.
* @deprecated Use the {@link RadioButtonGroup} component instead.
Copy link
Member Author

Choose a reason for hiding this comment

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

Trying out this new JSDoc syntax. In my editor it's just displayed as plain text, but ideally it links to the type definition of the RadioButtonGroup component.

@connor-baer
Copy link
Member Author

@robinmetral Requesting an early review from you to get some feedback on the changes if you have time before you leave. The only thing missing are the rewritten/expanded unit tests.

I plan to release a canary version of these changes to test that there are no inadvertent breaking changes.

@connor-baer connor-baer added this to the v6.x milestone May 17, 2023
Copy link
Contributor

@robinmetral robinmetral left a comment

Choose a reason for hiding this comment

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

I really really like the improvements here!! Super nice to align component APIs, docs, stories. Thank you! 💯

Is this the one that you were thinking of releasing as a canary? While it looks fine to me I think it might be worth testing in an app before the stable release 🙂

- **Don not** rely on tooltips to explain a checkbox
- **Do not** use commas or semicolons at the end of each line
- **Do** accompany the checkbox with a descriptive label
- **Do** write the label in a positive manner (e.g. _"Send me reports"_ instead of _"Do not send me reports"_)
Copy link
Contributor

Choose a reason for hiding this comment

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

We should review these with Frank at some point 😬 but fine to keep in here for now

Side note: I started to move away from the do/don't bullet points in some new docs (I usually moved the points to relevant sections, e.g. accessibility, or component variations). At some point it might make sense to rethink structure for the docs and make it consistent across components

@connor-baer connor-baer marked this pull request as ready for review May 22, 2023 14:57
@connor-baer connor-baer requested a review from a team as a code owner May 22, 2023 14:57
@connor-baer connor-baer requested review from tareqlol and removed request for a team May 22, 2023 14:57
connor-baer added a commit that referenced this pull request May 22, 2023
Squashed commit of the following:

* Clarify label docs
* Add tests for empty options
* Refactor test suite
* Update packages/circuit-ui/components/CheckboxGroup/CheckboxGroup.mdx
* Align options prop type
* Only log deprecation warning outside group components
* Revise the boolean input docs
* Uncontrolled boolean inputs
* Use field atoms in CheckboxGroup
* Deprecate the RadioButton and Selector components
* Align SelectorGroup with other boolean inputs
@connor-baer connor-baer merged commit 9aabce8 into main May 22, 2023
@connor-baer connor-baer deleted the feature/selectorgroup-validation branch May 22, 2023 15:57
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