<SegmentedPicker />: Use Radix UI's Tabs component under the hood, and rename to <Tabs />
#1543
Conversation
|
|
Visit the preview URL for this PR (updated for commit fbc96fc): https://penumbra-ui-preview--pr1543-jessepinho-tabs-v2-ap0r1mcc.web.app (expires Tue, 30 Jul 2024 22:55:36 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: 709d729610ef7a6369b23f1cb2b820a60cc685b1 |
| &:focus-within { | ||
| outline: none; | ||
| } | ||
|
|
||
| &:focus-within::after { |
There was a problem hiding this comment.
Radix UI's <Tabs /> uses focus-within rather than focus, so this addresses that change.
jessepinho
changed the title
jessepinho
changed the title
<SegmentedPicker />: Use Radix UI's Tabs component under the hood
jessepinho
changed the title
<SegmentedPicker />: Use Radix UI's Tabs component under the hood<SegmentedPicker />: Use Radix UI's Tabs component under the hood, and rename to <Tabs />
| */ | ||
| value: ValueType; | ||
| export interface TabsTab { | ||
| value: string; |
There was a problem hiding this comment.
Radix UI's <Tabs /> only supports strings as the value type.
| @@ -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} />; | |||
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
This PR addresses @VanishMax 's comment about accessibility issues with the
<SegmentedPicker />component. To address the issues, I refactored<SegmentedPicker />to use Radix UI's fully accessible<Tabs />component under the hood. Per Sam's agreement, I also renamed this component to<Tabs />.This component may be modified further once it's being used in actual pages, so this is just a gradual improvement for now.