-
Notifications
You must be signed in to change notification settings - Fork 2k
Web: add label clickable button with hover state #62046
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
b107bc1 to
deb85ce
Compare
deb85ce to
14ab9c7
Compare
| export function LabelButtonWithIcon({ | ||
| Icon, | ||
| placement, | ||
| children, | ||
| ...labelProps | ||
| }: LabelProps & { | ||
| Icon: React.ComponentType<IconProps>; | ||
| placement: IconPlacement; | ||
| children: React.ReactNode; | ||
| onClick?: () => void; | ||
| }) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Use PropsWithChildren.
| export function LabelButtonWithIcon({ | |
| Icon, | |
| placement, | |
| children, | |
| ...labelProps | |
| }: LabelProps & { | |
| Icon: React.ComponentType<IconProps>; | |
| placement: IconPlacement; | |
| children: React.ReactNode; | |
| onClick?: () => void; | |
| }) { | |
| export function LabelButtonWithIcon({ | |
| Icon, | |
| placement, | |
| children, | |
| ...labelProps | |
| }: { | |
| Icon: React.ComponentType<IconProps>; | |
| placement: IconPlacement; | |
| onClick?: () => void; | |
| } & LabelProps & PropsWithChildren) { |
| ); | ||
|
|
||
| return ( | ||
| <LabelWithHoverAffect {...labelProps} withHoverState> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Support keyboard navigation.
| <LabelWithHoverAffect {...labelProps} withHoverState> | |
| <LabelWithHoverAffect {...labelProps} withHoverState tabIndex={0}> |
Also, consider adding :focus and/or :active styles.
| Icon: React.ComponentType<IconProps>; | ||
| placement: IconPlacement; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To improve flexibility and promote reusability, I'd provide IconLeft and IconRight props instead of a single placement.
| if (resourceLabelConfig?.Icon) { | ||
| return ( | ||
| <StyledLabelButton | ||
| key={i} | ||
| {...sharedProps} | ||
| Icon={resourceLabelConfig.Icon} | ||
| placement={resourceLabelConfig.iconPlacement ?? 'right'} | ||
| > | ||
| {labelText} | ||
| </StyledLabelButton> | ||
| ); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Feels a little strange to base this logic on the presence of an icon. Can it be based on the presence of the onClick prop instead, which should use the hover state?
| const { formatCreateLabel, ...styles } = others; | ||
| const { formatCreateLabel, filterOption, ...styles } = others; | ||
| return ( | ||
| <FieldSelectWrapper {...wrapper} {...styles}> | ||
| <SelectCreatable<Opt, IsMulti, Group> | ||
| {...base} | ||
| filterOption={filterOption} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change seem a little out of place in this PR. Is it related in some way?
| export type ResourceLabelConfig = { | ||
| /** | ||
| * If defined, LabelButtonWithIcon component is used that | ||
| * renders icon and applies button hover states. | ||
| * | ||
| * Default is to use Label component without any hover states. | ||
| */ | ||
| Icon?: React.ComponentType<IconProps>; | ||
| /** | ||
| * If field "Icon" is defined, default renders the icon to the right. | ||
| */ | ||
| iconPlacement?: IconPlacement; | ||
| /** | ||
| * Default label kind is "secondary". | ||
| */ | ||
| kind?: LabelKind; | ||
| }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit
| export type ResourceLabelConfig = { | |
| /** | |
| * If defined, LabelButtonWithIcon component is used that | |
| * renders icon and applies button hover states. | |
| * | |
| * Default is to use Label component without any hover states. | |
| */ | |
| Icon?: React.ComponentType<IconProps>; | |
| /** | |
| * If field "Icon" is defined, default renders the icon to the right. | |
| */ | |
| iconPlacement?: IconPlacement; | |
| /** | |
| * Default label kind is "secondary". | |
| */ | |
| kind?: LabelKind; | |
| }; | |
| export type ResourceLabelConfig = Omit<ComponentProps<typeof LabelButtonWithIcon>, 'children'>; |
part of https://github.com/gravitational/teleport.e/issues/7183
Created a label component that accepts any icon that can be rendered on left or right, and have same hover state as buttons depending on
kindAlso adds customizing how resource labels are rendered for unified resources
Story for labels: https://localhost:9002/?path=/story/design-label--labels
Story for unified resource: https://localhost:9002/?path=/story/shared-unifiedresources-items--cards
figma: https://www.figma.com/design/CzxaBHF8hirrYv2bTVa4Rw/Identity-Governance?node-id=8638-79995&t=ujyuv9gqrjGLQWfV-0