-
Notifications
You must be signed in to change notification settings - Fork 34
feat(#3070): v2 badge with emphasis and size #3097
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: v2-2998-coded-component-updates
Are you sure you want to change the base?
feat(#3070): v2 badge with emphasis and size #3097
Conversation
633ba13 to
a2f80e9
Compare
39716fb to
964e764
Compare
| setTimeout(() => validateType(type), 1); | ||
| setTimeout(() => { | ||
| if (version === "2" && !type) { | ||
| type = "default"; |
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.
Type is required in v1 but optional in v2. I'm not sure if this is the best approach. Maybe we could handle it in the wrappers or rethink how to have a default option.
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.
should we keep it required in both? I know we have a default badge type, but I don't know if that's expected.
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.
@bdfranck Looks great overall. Just a few spots where we can use tokens instead of hardcoded values, and a couple typos for the token names.
| setTimeout(() => validateType(type), 1); | ||
| setTimeout(() => { | ||
| if (version === "2" && !type) { | ||
| type = "default"; |
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.
should we keep it required in both? I know we have a default badge type, but I don't know if that's expected.
|
@twjeffery I've amended my commit with the following changes:
|
964e764 to
b71f24f
Compare
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 PR updates the badge component to match the v2 styles and adds two new properties: size and emphasis.