Segmented Control #591
Segmented Control #591
Conversation
🦋 Changeset detectedLatest commit: 34855e1 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
|
@marcelines is attempting to deploy a commit to the Status Team on Vercel. A member of the Team first needs to authorize it. |
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
packages/components/src/segmented-control/segmented-control.tsx
Outdated
Show resolved
Hide resolved
packages/components/src/segmented-control/segmented-control.tsx
Outdated
Show resolved
Hide resolved
packages/components/src/segmented-control/segmented-control.tsx
Outdated
Show resolved
Hide resolved
|
This should be implemented using https://www.radix-ui.com/primitives/docs/components/toggle-group#examples, don't you think? |
| size, | ||
| type, |
There was a problem hiding this comment.
passing this shouldn't be necessary, with context
| ref: (el: HTMLButtonElement | null) => { | ||
| segmentRefs.current[index] = el | ||
| }, | ||
| active: child.props.value === props.value, |
There was a problem hiding this comment.
i am sure radix has a data-attribute for active toggle
|
|
||
| const TabsContext = createContext<Pick<RootProps, 'size' | 'type'>>({}) | ||
|
|
||
| export const Root = (props: RootProps) => { |
There was a problem hiding this comment.
please follow structure of other components (see Tabs, DropdownMenu), e.g.
- Root at the top
- styles close to the used components instead of all at the top
| } | ||
| >( | ||
| ( | ||
| { children, emoji, active, size = '32', type = 'grey', ...itemProps }, |
There was a problem hiding this comment.
consume type and size from context, that's why you added it, no?
| } | ||
| >( | ||
| ( | ||
| { icon, children, active, size = '32', type = 'grey', ...itemProps }, |
There was a problem hiding this comment.
consume type and size from context, that's why you added it, no?
|
Next time you update the composition (like adding context), think it through. Adding React Context eliminates the need for these cloning gymnastics. |
You're absolutely right. I'm tired and i missed it. But i still need it to be cloned so that i can pass the ref to have the animated selector. |
| @@ -42,7 +52,7 @@ export const List = forwardRef< | |||
| React.ElementRef<typeof Tabs.List>, | |||
| React.ComponentPropsWithoutRef<typeof Tabs.List> | |||
| >((props, ref) => { | |||
| const { size } = useContext(TabsContext)! | |||
| const { size } = useTabsContext() | |||
| // Ensuring there is always a value | ||
| // @see https://www.radix-ui.com/primitives/docs/components/toggle-group#ensuring-there-is-always-a-value | ||
| if (value) { | ||
| onValueChange(value) | ||
| } |
Jenkins BuildsClick to see older builds (1)
|
|
I see you've already checked the changes @marcelines 👍. I made a few cosmetic changes to align with other components in the library. Reordering, renaming from "tab" to "segment"... I removed the distinction between Added hook to consume context to this component and |
There was a problem hiding this comment.
non-blocking: this story appears to have a lot of copy-pasting instead of doing a better abstraction.
There are three identical columns, each contained within a different background. Why not extract the column consisting of 10 components into a single "variant" and render the "variant" three times with different backgrounds?
There was a problem hiding this comment.
Yes. Much better. 👍
Changes
componentspackage along with its story.