feat(ui): chonkify segmentedControl #89589
Conversation
…y-segmentedControl # Conflicts: # static/app/components/core/segmentedControl/index.tsx
…y-segmentedControl
…y-segmentedControl
github-actions
bot
added
the
Scope: Frontend
| export function getChonkButtonStyles( | ||
| p: ButtonProps & {theme: DO_NOT_USE_ChonkTheme} | ||
| p: Pick<ButtonProps, 'size' | 'priority' | 'busy' | 'disabled' | 'borderless'> & { | ||
| theme: DO_NOT_USE_ChonkTheme; | ||
| } |
There was a problem hiding this comment.
changed the interface here to be as minimal as possible - only what gets really used, so that I can re-use it in the segmentgedControl
| @@ -150,7 +152,7 @@ export function getChonkButtonStyles( | |||
| }, | |||
| }, | |||
|
|
|||
| '&:active, &[aria-expanded="true"]': { | |||
| '&:active, &[aria-expanded="true"], &[aria-checked="true"]': { | |||
There was a problem hiding this comment.
segmentedControl is a checkbox and thus sets aria-checked for “active” elements
| @@ -159,7 +161,7 @@ export function getChonkButtonStyles( | |||
| }, | |||
| }, | |||
|
|
|||
| '&:disabled': { | |||
| '&:disabled, &[aria-disabled="true"]': { | |||
There was a problem hiding this comment.
I’ve set aria-disabled for the element in the segmentedControl that receives styling, so I need to match it here, too.
| ${p => p.theme.buttonPadding[p.size]} | ||
| font-weight: ${p => p.theme.fontWeightNormal}; | ||
|
|
||
| ${p => ({...getChonkButtonStyles({...p, disabled: p.isDisabled, priority: p.isSelected && p.priority === 'primary' ? 'primary' : 'default'})})} |
There was a problem hiding this comment.
took me a while to figure out why disabled styles don’t work. disabled vs isDisabled, and no typescript errors because the prop is optional and there is no excess property checking 😭
There was a problem hiding this comment.
Oh my god... we really need to delete these dual props, this is a waste of time + having isDisabled and disabled is just confusing
There was a problem hiding this comment.
I think we use isDisabled a lot because of “boolean props should start with is”, which is okay, but then we have disabled because the html property is disabled and this gets forwarded to inputs :/
let’s put it on the list to decide for one
| // Once an item is selected, it gets a heavier font weight and becomes slightly | ||
| // wider. To prevent layout shifts, we need a hidden container (HiddenLabel) that | ||
| // will always have normal weight to take up constant space; and a visible, | ||
| // absolutely positioned container (VisibleLabel) that doesn't affect the layout. |
There was a problem hiding this comment.
none of this madness is needed in chonk because selected elements don’t get wider so we can get rid of those wrappers and the absolute positioning 🎉
also we need a special handling for when there's only two elements in the segmentedControl
| ${({theme}) => theme.focusRing}; | ||
| } | ||
|
|
||
| ${p => p.isSelected && `z-index: 1;`} |
There was a problem hiding this comment.
Can we move this above under L53 and just do something like z-index: isSelected ? 1 : undefined so that the base rules are somewhat centralized? I hate it when I need to use search inside a css files just to find that one of the rules is defined somewhere totally random, and I've also seen instances where these end up redefining a z-index somewhere else, which also results in duplicate css rules
There was a problem hiding this comment.
totally: 4c010d8
this is still a leftover from the existing styles, I don’t even know why we’d need it 🤷♂️
| priority={priority} | ||
| data-test-id={props.value} | ||
| aria-checked={isSelected} | ||
| aria-disabled={isDisabled} |
No description provided.