-
Notifications
You must be signed in to change notification settings - Fork 597
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: update badge API and styles #3147
Conversation
48a301b
to
9dac442
Compare
private appearanceChanged( | ||
oldValue: BadgeAppearance, | ||
newValue: BadgeAppearance | ||
): void { | ||
if (oldValue !== newValue) { |
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 class should be applied through the template or this can cause a runtime error when this is set during construction
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.
Are there alternatives? This isn't global across all badges and ideally we'd reuse the template.
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.
Leveraging DOM.queueUpdate() here
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.
Just one little logical thing that may need to be addressed.
import { Badge, BadgeTemplate as template } from "@microsoft/fast-foundation"; | ||
import { BadgeStyles as styles } from "./badge.styles"; | ||
|
||
export type BadgeFill = "accent" | "lightweight" | "neutral" | "string"; | ||
export type BadgeAppearance = "accent" | "lightweight" | "neutral" | string; |
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.
Good catch on "string"
style="background-color: var(--badge-fill-${x => | ||
x.fill}); color: var(--badge-color-${x => x.color})" | ||
style="${x => | ||
x.fill || x.color |
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.
Is it possible that there's a fill but no color or a color but no fill?
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.
Yeah, this isn't ideal. I think we can update but I wanted to mitigate the issue of this being applied when neither was used. I think it's less egregious to have one undefined variable here that's not in use than both. We should fix this though to have both only written if they exist.
* feat: update badge API and styles * add initial package version BREAKING CHANGE
* feat: update badge API and styles * add initial package version BREAKING CHANGE
* feat: update badge API and styles * add initial package version BREAKING CHANGE
* feat: update badge API and styles * add initial package version BREAKING CHANGE
* feat: update badge API and styles * add initial package version BREAKING CHANGE
* feat: update badge API and styles * add initial package version BREAKING CHANGE
* feat: update badge API and styles * add initial package version BREAKING CHANGE: fundamentally changes and breaks the badge component API and styles
Description
This PR udpates the badge template to conditionally render the CSS variable map values when one is actually supplied. Additionally, this updates the base styles of Badge in fast-components and adds appearances to
fast-components-msft
.Issue type checklist
Is this a breaking change?
Process & policy checklist