Add RcButtonSplit to the component library#17055
Conversation
Signed-off-by: Phillip Rak <rak.phillip@gmail.com>
Signed-off-by: Phillip Rak <rak.phillip@gmail.com>
Signed-off-by: Phillip Rak <rak.phillip@gmail.com>
Signed-off-by: Phillip Rak <rak.phillip@gmail.com>
codyrancher
left a comment
There was a problem hiding this comment.
I reviewed the code more than I did the styling since I suspect @oboc-sts will do a better job than I will but I can follow up on that if desired.
| size?: ButtonSize; | ||
| // eslint-disable-next-line vue/require-default-prop | ||
| ariaLabel?: string; | ||
| placement?: Placement; |
There was a problem hiding this comment.
Just a preference so leave it if you prefer otherwise. I think I'd rather just see undefined defaults over eslint disables.
| describe('variant prop', () => { | ||
| it.each([ | ||
| ['primary', 'variant-primary'], | ||
| ['secondary', 'variant-secondary'], |
There was a problem hiding this comment.
Missing multiAction variant from the test and I'm sure it's because this is a dropdown but does that mean we should prevent it from being used as a prop value?
There was a problem hiding this comment.
Based on feedback from Ovi, I think that we will drop link, ghost, and multiAction
| @@ -0,0 +1 @@ | |||
| export { default as RcButtonSplit } from './RcButtonSplit.vue'; | |||
There was a problem hiding this comment.
Do we also want to add an export to pkg/rancher-components/src/index.ts?
There was a problem hiding this comment.
Yes, very much so. This has been fixed.
| import { ButtonVariant, ButtonSize, IconProps } from '@components/RcButton/types'; | ||
| import type { Placement } from 'floating-vue'; | ||
|
|
||
| withDefaults(defineProps<{ |
There was a problem hiding this comment.
Do we want a disabled prop?
There was a problem hiding this comment.
Yes, I think this is important. Fixed
|
|
||
| <template #dropdownCollection> | ||
| <RcDropdownItem | ||
| v-for="(label, id) in items" |
There was a problem hiding this comment.
Yes, that's a good catch! Fixed
There was a problem hiding this comment.
I'll see what we can do to maybe resize the canvas in storybook. Otherwise, I don't think we will be in control of the overflow.
There was a problem hiding this comment.
This should be fixed now.. let me know what you think
| :deep(.rc-button-split-trigger.variant-primary), | ||
| :deep(.rc-button-split-trigger.variant-secondary), | ||
| :deep(.rc-button-split-trigger.variant-tertiary) { | ||
| border-left: 1px solid rgba(255, 255, 255, 0.3); |
There was a problem hiding this comment.
I'll sync with Ovi to better understand how we might want to handle tertiary buttons
|
|
||
| <template> | ||
| <RcDropdown | ||
| :aria-label="ariaLabel" |
There was a problem hiding this comment.
ariaLabel is forwarded to RcDropdown which labels the dropdown menu.
Do we want to label the group with a role and aria-label.
There was a problem hiding this comment.
I think that we might want 3 potential labels for this component:
- Rename this aria label for the dropdown, which is properly applied to the RcDropdown component
- Add a label for the primary button - this might be the default
ariaLabelprop - Add a label for the trigger
oboc-sts
left a comment
There was a problem hiding this comment.
Looking good @rak-phillip 💪
Based on the static screenshots alone, I only have 2+1 concerns:
- Styling wise, the text line-height used on the main button seems to push the label of the button down a bit (it's not optically align to middle)
- We should only support the "split" variant for
primary,secondaryand maybe (maybe)tertiary, not for all button types (link and ghost should definitely NOT support this). Let's talk about thetertiaryoffline - I need to better understand where we're using it right now to assess it's usefulness in having the new split variant. - Actions popover alignment. Right now, it seems that we only support aligning the actions popover to the left of the split button - this works nicely for actions with really short names (shorter than the action label used on the main button). But, if the actions in the popover are longer... we'd want to consider aligning the popover to the RHS of the split button to ease the scanning effort. However, this would have to the into account the "bouncing off" the screen edge. So it's a bit more complex than it appears. As a compromise, we can leave this improvement for the next iteration of the component.
Wdyt?
Signed-off-by: Phillip Rak <rak.phillip@gmail.com>
Signed-off-by: Phillip Rak <rak.phillip@gmail.com>
Signed-off-by: Phillip Rak <rak.phillip@gmail.com>
Signed-off-by: Phillip Rak <rak.phillip@gmail.com>
Signed-off-by: Phillip Rak <rak.phillip@gmail.com>
Signed-off-by: Phillip Rak <rak.phillip@gmail.com>
Signed-off-by: Phillip Rak <rak.phillip@gmail.com>
Signed-off-by: Phillip Rak <rak.phillip@gmail.com>
codyrancher
left a comment
There was a problem hiding this comment.
Good from my perspective.
Signed-off-by: Phillip Rak <rak.phillip@gmail.com>




Summary
This adds RcButtonSplit to the component library. This component reflects the design system usage described in the Buttons & Links section.
Occurred changes and/or fixed issues
itemsprop to RcButtonSplitTechnical notes summary
The design system co-mingles the button and the dropdown button. I'm keeping these as separate components to reduce the amount of complexity in the base
RcButtoncomponent. I also opted for the naming convection ofRcButtonSplitto keep the naming in close proximity to other button components likeRcButtonandButtonDropdown.Areas or cases that should be tested
We can test adding this to a few areas where we might have resorted to workarounds to ensure that this new component covers all of our existing use cases.
Areas which could experience regressions
NA - This is purely additive at this point.
Screenshot/Video
Checklist
Admin,Standard UserandUser Base