feat: add segmented control radio and tabs - #1101
Conversation
|
@frankieyan before I send this off to review, it has been a while so wanted to check our process on this. I haven't removed the current Reactist Tabs from in here yet. So currently both exist:
This is largely to not break consumers. Would we mark as deprecated, then once this is merged, and OUR consumers are on the latest component, then completely remove? |
Yeah! I think marking the old tabs as deprecated first would be the way to go. I'd go with adding a badge to the stories: reactist/.storybook/badges/README.md Line 22 in eeac516 And marking the components themselves as deprecated with jsdoc: https://jsdoc.app/tags-deprecated But not rename the button to avoid a breaking change for now (but of course if you'd like to offload the renaming in consumers or even migrate right way to an agent, that's fine too!) |
|
Thanks @frankieyan, ended up doing this. And I've added you as second reviewer on this one, hope that's OK. |
doistbot
left a comment
There was a problem hiding this comment.
This PR adds SegmentedControlRadio and SegmentedControlTabs components to Reactist, moves the compound Tabs onto shared segmented-control styling, and deprecates the old Tabs API with migration notes.
I also included a few optional follow-up notes in the details below.
Optional follow-up notes (4)
src/segmented-control/segmented-control.tsx:185: The
@deprecatedJSDoc says "consumers of the initial Reactist port can migrate without a breaking change." Since this is the first timeSegmentedControlappears in Reactist, there are no existing consumers. The comment should describe why the alias exists (e.g., to match the name used during development in Todoist via yalc) rather than implying pre-existing external usage.src/segmented-control/segmented-control.test.tsx:17:
flushAnimationFrames(and the near-identicalrenderSegmentedControl/renderSegmentedControlTabswrappers) are duplicated verbatim insegmented-control.test.tsxandsegmented-control-tabs.test.tsx. Reactist already centralizes shared test utilities insrc/utils/test-helpers.tsx(e.g.flushMicrotasks). Consider extractingflushAnimationFramesthere so both files import it, matching the existing pattern and avoiding drift between the two copies.src/segmented-control/segmented-control-tabs.tsx:47:
ControlledTabsSelectionPropsandUncontrolledTabsSelectionProps(L47-57) are structurally identical toControlledSelectionProps/UncontrolledSelectionPropsinsegmented-control.tsx(L54-64). Since this file already importsAriaLabelPropsfrom./segmented-control, consider exporting and reusing the base controlled/uncontrolled types instead of redefining them.src/segmented-control/segmented-control.tsx:114: The
justifyContentAlignMaplookup is defined identically here and intabs.tsx(TabList, line 207). Both map the same'start' | 'center' | 'end'→BoxJustifyContentvalues for the samealignprop. Extracting a shared helper (e.g. inutils/common-typesor a small shared module) would prevent the two from drifting if the alignment options change.
|
Addressed the Doistbot review feedback in f8ee8fe:
The PR-size suggestion was not applied: the radio/tabs APIs and their shared styling are one cohesive migration, while splitting now would make their dependency relationship harder to review and land. Validation: full React 19 and React 18 suites pass (46 suites, 631 passed and 1 skipped per version), along with lint, both type-checks, and React Compiler tracking. |
|
|
||
| export const FullWidthWithWrappingLabels: Story = { | ||
| render: () => ( | ||
| <Box width="small"> |
There was a problem hiding this comment.
This could be limiting the full width
|
|
||
| export const FullWidth: Story = { | ||
| render: () => ( | ||
| <Box width="small"> |
There was a problem hiding this comment.
Same here, potentially it limits the full width
| @@ -0,0 +1,14 @@ | |||
| export type { | |||
| AriaDescriptionProps, | |||
There was a problem hiding this comment.
My original reaction was that the name is too generic, but then I checked and it actually describes it very well. But I feel like we should move it somewhere else, maybe in src/utils/common-types.ts?
| background-color: var(--reactist-tab-themed-disabled-fill); | ||
| color: var(--reactist-tab-themed-disabled-tint); | ||
| .radio-item > input { | ||
| clip: rect(0 0 0 0); |
There was a problem hiding this comment.
I think we should preserve the original comment:
/* ref: https://www.a11yproject.com/posts/how-to-hide-content/ */|
Something I didn't realize with the SegmentedControl is that it's still using Ariakit 😅 so this would be a lateral move for us. On the bleeding issue Seva pointed out, it also happens in the developer options, but I thought that was still using the old Tabs - which isn't actually better because it does this:
Would it be worth pivoting to the BaseUI Tabs and RadioGroup instead? In small viewports, I wonder if we could adopt the pattern that the template gallery uses so that the control is scrollable/draggable:
|





Short description
Adds the Todoist segmented-control patterns to Reactist while retaining the existing compound Tabs API for compatibility.
SegmentedControlRadioandSegmentedControlTabscomponents with controlled and uncontrolled selection.SegmentedControlTabs.Locally verified through Reactist Storybook and in Todoist Web via
yalc.References
Demo
Old one deprecated
PR Checklist