-
Notifications
You must be signed in to change notification settings - Fork 15
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
feat: ui.badge #973
feat: ui.badge #973
Conversation
import React from 'react'; | ||
import { | ||
Badge as DHCBadge, | ||
BadgeProps as DHCBadgeProps, | ||
} from '@deephaven/components'; | ||
|
||
export function Badge(props: DHCBadgeProps): JSX.Element { | ||
const { variant } = props; | ||
return ( | ||
// eslint-disable-next-line react/jsx-props-no-spreading | ||
<DHCBadge {...props} variant={variant} /> | ||
); | ||
} | ||
|
||
Badge.displayName = 'Badge'; | ||
|
||
export default Badge; |
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.
Right now this wrapper isn't doing anything, and for some reason you're pulling variant
out of the props just to pass it right into DHCBadge (which is unnecessary, since it was in props and you're passing all of them through already).
We do want to wrap text children though as per my other comments:
import React from 'react'; | |
import { | |
Badge as DHCBadge, | |
BadgeProps as DHCBadgeProps, | |
} from '@deephaven/components'; | |
export function Badge(props: DHCBadgeProps): JSX.Element { | |
const { variant } = props; | |
return ( | |
// eslint-disable-next-line react/jsx-props-no-spreading | |
<DHCBadge {...props} variant={variant} /> | |
); | |
} | |
Badge.displayName = 'Badge'; | |
export default Badge; | |
import React from 'react'; | |
import { | |
Badge as DHCBadge, | |
BadgeProps as DHCBadgeProps, | |
} from '@deephaven/components'; | |
import { wrapTextChildren } from './utils'; | |
export function Badge(props: DHCBadgeProps): JSX.Element { | |
const { children } = props; | |
return ( | |
// eslint-disable-next-line react/jsx-props-no-spreading | |
<DHCBadge {...props}>{wrapTextChildren(children)}</DHCBadge> | |
); | |
} | |
Badge.displayName = 'Badge'; | |
export default Badge; |
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 is on spectrum, but that sub-set of colors supported by variant is pretty weird. I wouldn't be opposed to expanding the implementation to manually pass in variant as backgroundColor UNSAFE_style then accept any color. We would also have to pick a proper contrasting fg color though.
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.
@dsmmcken Would this be something we wanted to look into more concretely?
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.
Unsure if we should hold this until that, or follow ticket. Because it would be a breaking change.
@mofojed thoughts?
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.
Can be done in a follow up ticket. Doesn't have to be a breaking change, could type as variant: BadgeVariant | Color | None
, and check if it's one of the built-in variants before falling back to apply as a colour (in JS).
45f92fc
Closes #935 and Closes #868
(Also includes docs for ui.badge).