feat(component): add ButtonGroup component #556
Conversation
|
|
||
| const renderedDropdown = useMemo( | ||
| () => ( | ||
| <StyledFlexItem data-testid="button-group-dropdown" isVisible={isMenuVisible} ref={dropdownRef} role="listitem"> |
There was a problem hiding this comment.
About data-testid, these are the only reliable way we could think of for targeting them and setting the offsetWidths so that we could test the collapsing functionality (jsdom does not actually render things, so all elements have an offsetWidth of 0 in the testing environment)
There was a problem hiding this comment.
Yeah, that was the same issue we ran into with the PillsTabs.
🍹 I'm :gucci: with this, but could we align it with a similar name to the PillsTabs one? Definitely a nitpick:
| <StyledFlexItem data-testid="button-group-dropdown" isVisible={isMenuVisible} ref={dropdownRef} role="listitem"> | |
| <StyledFlexItem data-testid="buttongroup-dropdown-toggle" isVisible={isMenuVisible} ref={dropdownRef} role="listitem"> |
There was a problem hiding this comment.
Thank you for your comment! Sure, I agree with you!
Done ✅
| &:focus { | ||
| z-index: 1; | ||
| } |
There was a problem hiding this comment.
I use z-index: 1 for correct displaying of box-shadow (pls see the example below). I decide to not use the value from theme cause it will be not semantic for my case and I think creating a new one will be redundant.
Without z-index
With z-index
| [...actionsState] | ||
| .reverse() | ||
| .sort(({ isVisible }) => (isVisible ? -1 : 1)) |
There was a problem hiding this comment.
Here I'm sorting elements that visible elements will be first in the row. I do this for the correct working of pseudo-class :first-of-type which controls border-radius for the left side of the first element.
it's the possible case when the first element will be hidden under ellispsis when this element will have actionType="destructive".
| export interface ButtonGroupAction extends Omit<ButtonProps, 'children' | 'iconOnly' | 'iconRight' | 'iconLeft'> { | ||
| text: string; | ||
| icon?: React.ReactElement; | ||
| } |
There was a problem hiding this comment.
Initially, Button has 'iconOnly' | 'iconRight' | 'iconLeft' properties but, according to the guidelines I can show the icon only when action will be hidden under ellipsis. So, I decide to exclude 'iconOnly' | 'iconRight' | 'iconLeft' and just add the icon property.
| border-top-left-radius: ${({ theme }) => theme.borderRadius.normal}; | ||
| } | ||
|
|
||
| &:nth-last-of-type(-n + 2) ${StyledButton} { |
There was a problem hiding this comment.
I use &:nth-last-of-type(-n + 2) instead of :last-of-type according to case when dropdown is visible and when some of actions has actionType="destructive" and all action are fit... so, it sets border-radius to dropdown toggle and the last element of action. And it's the impossible case when last action and dropdown toggle will have border-radius at the same time case I have sorting (pls see comment above)
| const excludeIconProps = <T extends any>({ | ||
| iconOnly, | ||
| iconRight, | ||
| iconLeft, | ||
| ...actionProps | ||
| }: T): Pick<T, Exclude<keyof T, 'iconOnly' | 'iconRight' | 'iconLeft'>> => actionProps; |
There was a problem hiding this comment.
Let's not complicate this with generics:
| const excludeIconProps = <T extends any>({ | |
| iconOnly, | |
| iconRight, | |
| iconLeft, | |
| ...actionProps | |
| }: T): Pick<T, Exclude<keyof T, 'iconOnly' | 'iconRight' | 'iconLeft'>> => actionProps; | |
| const excludeIconProps = ({ | |
| iconOnly, | |
| iconRight, | |
| iconLeft, | |
| ...actionProps | |
| }: ButtonProps & { text: string }): ButtonGroupAction => actionProps; |
I don't really like the & { text: string } part 😅 , but I guess it's okay since it's an internal implementation 🤷
There was a problem hiding this comment.
Thank you! I lil bit changed your suggestion from { text: string } to Pick<ButtonGroupAction, 'text'>, something like single source of truth for the text prop 😅
|
|
||
| const renderedDropdown = useMemo( | ||
| () => ( | ||
| <StyledFlexItem data-testid="button-group-dropdown" isVisible={isMenuVisible} ref={dropdownRef} role="listitem"> |
There was a problem hiding this comment.
Yeah, that was the same issue we ran into with the PillsTabs.
🍹 I'm :gucci: with this, but could we align it with a similar name to the PillsTabs one? Definitely a nitpick:
| <StyledFlexItem data-testid="button-group-dropdown" isVisible={isMenuVisible} ref={dropdownRef} role="listitem"> | |
| <StyledFlexItem data-testid="buttongroup-dropdown-toggle" isVisible={isMenuVisible} ref={dropdownRef} role="listitem"> |
| actions.map((action) => ({ isVisible: true, action: excludeIconProps(action), ref: createRef<HTMLDivElement>() })), | ||
| ); | ||
|
|
||
| const changeActionsVisibility = useCallback(() => { |
There was a problem hiding this comment.
🍹 nitpick, but could we align this to a similar name to the PillsTabs one?
| const changeActionsVisibility = useCallback(() => { | |
| const hideOverflowedActions = useCallback(() => { |
There was a problem hiding this comment.
Sure, thank you, done ✅
What?
Add the
ButtonGroupcomponent.Examples
Docs
Testing/Proof
Unit tests.
ping @chanceaclark @jorgemoya