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

<SegmentedPicker />: Use Radix UI's Tabs component under the hood, and rename to <Tabs /> #1543

Merged
merged 5 commits into from
Jul 26, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -1,11 +1,10 @@
import type { Meta, StoryObj } from '@storybook/react';
import { useArgs } from '@storybook/preview-api';

import { SegmentedPicker } from '.';
import { Tabs } from '.';

const meta: Meta<typeof SegmentedPicker> = {
component: SegmentedPicker,
title: 'SegmentedPicker',
const meta: Meta<typeof Tabs> = {
component: Tabs,
tags: ['autodocs', '!dev'],
argTypes: {
value: { control: false },
Expand All @@ -15,7 +14,7 @@ const meta: Meta<typeof SegmentedPicker> = {
};
export default meta;

type Story = StoryObj<typeof SegmentedPicker>;
type Story = StoryObj<typeof Tabs>;

export const Basic: Story = {
args: {
Expand All @@ -34,6 +33,6 @@ export const Basic: Story = {

const onChange = (value: { toString: () => string }) => updateArgs({ value });

return <SegmentedPicker {...props} onChange={onChange} />;
return <Tabs {...props} onChange={onChange} />;
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion (non-blocking, worth implementing): expose TabsContent component from Radix and somehow allow the children for Tabs.

If you inspect the markup of the Radix Tabs example, you can see that Tabs.Content components are nested inside the Tabs.Root. It allows Radix to set useful attributes like data-state="active" and role="tabpanel". Plus, there is a rendering logic involved that all [role="tabpanel"] divs are rendered but inactive divs have empty contents. All of this corresponds to W3C standards.

Developers might and probably will forget to implement this, so it would be useful to help them (and ourselves)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call — will look into it. I'm not sure that <TabsContent /> will be appropriate in our case, since we won't have multiple tab contents rendered on the page at the same time — <Tabs /> will usually be used for routing, I believe. But I created a ticket to look into it nonetheless.

},
};
Original file line number Diff line number Diff line change
@@ -1,12 +1,12 @@
import { describe, expect, it, vi } from 'vitest';
import { SegmentedPicker } from '.';
import { Tabs } from '.';
import { fireEvent, render } from '@testing-library/react';
import { ThemeProvider } from '../ThemeProvider';

describe('<SegmentedPicker />', () => {
describe('<Tabs />', () => {
it('renders a button for each of the `options`', () => {
const { queryByText } = render(
<SegmentedPicker
<Tabs
value='one'
options={[
{ label: 'One', value: 'one' },
Expand All @@ -24,7 +24,7 @@ describe('<SegmentedPicker />', () => {
it("calls the `onChange` handler with the clicked option's value when clicked", () => {
const onChange = vi.fn();
const { getByText } = render(
<SegmentedPicker
<Tabs
value='one'
options={[
{ label: 'One', value: 'one' },
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import { tab } from '../utils/typography';
import { motion } from 'framer-motion';
import { useId } from 'react';
import { buttonInteractions } from '../utils/button';
import * as RadixTabs from '@radix-ui/react-tabs';

const TEN_PERCENT_OPACITY_IN_HEX = '1a';

Expand All @@ -25,7 +26,7 @@ const outlineColorByActionType: Record<ActionType, keyof DefaultTheme['color']['
unshield: 'unshieldFocusOutline',
};

const SegmentButton = styled.button<{
const Tab = styled.button<{
$actionType: ActionType;
$getFocusOutlineColor: (theme: DefaultTheme) => string;
$getBorderRadius: (theme: DefaultTheme) => string;
Expand Down Expand Up @@ -68,37 +69,27 @@ const SelectedIndicator = styled(motion.div)`
z-index: -1;
`;

export interface SegmentedPickerOption<ValueType> {
/**
* The value to pass to the `onChange` handler when clicked. Must be unique
* across all segments, and must be either a string, number, or an object with
* a `.toString()` method so that it can be used as a React key.
*/
value: ValueType;
export interface TabsTab {
value: string;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Radix UI's <Tabs /> only supports strings as the value type.

label: string;
disabled?: boolean;
}

export interface SegmentedPickerProps<ValueType extends { toString: () => string }> {
/**
* The currently selected value. Will be compared to the `options`' `value`
* property using `===` to determine which segment is selected.
*/
value: ValueType;
onChange: (value: ValueType) => void;
options: SegmentedPickerOption<ValueType>[];
export interface TabsProps {
value: string;
onChange: (value: string) => void;
options: TabsTab[];
actionType?: ActionType;
}

/**
* Renders a segmented picker where only one option can be selected at a time.
* Functionally equivalent to a `<select>` element or a set of radio buttons,
* but looks nicer when you only have a few options to choose from. (Probably
* shouldn't be used with more than 5 options.)
* Use tabs for switching between related pages or views.
*
* Built atop Radix UI's `<Tabs />` component, so it's fully accessible and
* supports keyboard navigation.
*
* @example
* ```TSX
* <SegmentedPicker
* <Tabs
* value={value}
* onChange={setValue}
* options={[
Expand All @@ -109,29 +100,36 @@ export interface SegmentedPickerProps<ValueType extends { toString: () => string
* />
* ```
*/
export const SegmentedPicker = <ValueType extends { toString: () => string }>({
value,
onChange,
options,
actionType = 'default',
}: SegmentedPickerProps<ValueType>) => {
export const Tabs = ({ value, onChange, options, actionType = 'default' }: TabsProps) => {
const layoutId = useId();

return (
<Root>
{options.map(option => (
<SegmentButton
key={option.value.toString()}
onClick={() => onChange(option.value)}
$actionType={actionType}
disabled={option.disabled}
$getFocusOutlineColor={theme => theme.color.action[outlineColorByActionType[actionType]]}
$getBorderRadius={theme => theme.borderRadius.xs}
>
{value === option.value && <SelectedIndicator layout layoutId={layoutId} />}
{option.label}
</SegmentButton>
))}
</Root>
<RadixTabs.Root value={value} onValueChange={onChange}>
<RadixTabs.List asChild>
<Root>
{options.map(option => (
<RadixTabs.Trigger
value={option.value}
key={option.value.toString()}
disabled={option.disabled}
asChild
>
<Tab
onClick={() => onChange(option.value)}
disabled={option.disabled}
$actionType={actionType}
$getFocusOutlineColor={theme =>
theme.color.action[outlineColorByActionType[actionType]]
}
$getBorderRadius={theme => theme.borderRadius.xs}
>
{value === option.value && <SelectedIndicator layout layoutId={layoutId} />}
{option.label}
</Tab>
</RadixTabs.Trigger>
))}
</Root>
</RadixTabs.List>
</RadixTabs.Root>
);
};
7 changes: 6 additions & 1 deletion packages/ui/src/utils/button.ts
Original file line number Diff line number Diff line change
Expand Up @@ -31,10 +31,15 @@ const focusOutline = css<{
* disabled button, the overlay of the disabled button would be above the
* outline, making the outline appear to be partly cut off.
*/
&:focus::after {
&:focus-within {
outline: none;
}

&:focus-within::after {
Comment on lines +34 to +38
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Radix UI's <Tabs /> uses focus-within rather than focus, so this addresses that change.

outline-color: ${props => props.$getFocusOutlineColor(props.theme)};
}

&:disabled,
&:disabled::after {
pointer-events: none;
}
Expand Down
Loading