Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Changes from 4 commits
10462d5
01f002c
af081a1
61136c5
0e0fe00
e7ecbf1
45f92fc
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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:
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).