-
Notifications
You must be signed in to change notification settings - Fork 207
feat(badge): remove modifiers from the API [SWC-1264] #4283
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(badge): remove modifiers from the API [SWC-1264] #4283
Conversation
|
| Name | Type |
|---|---|
| @spectrum-css/alertdialog | Major |
| @spectrum-css/asset | Major |
| @spectrum-css/assetcard | Major |
| @spectrum-css/assetlist | Major |
| @spectrum-css/avatar | Major |
| @spectrum-css/badge | Major |
| @spectrum-css/miller | Major |
| @spectrum-css/well | Major |
| @spectrum-css/bundle | Patch |
| @spectrum-css/preview | Patch |
Click here to learn what changesets are, and how to add one.
Click here if you're a maintainer who wants to add a changeset to this PR
📚 Branch previewPR #4283 has been deployed to Azure Blob Storage: https://spectrumcss.z13.web.core.windows.net/pr-4283/index.html. |
File metricsSummaryTotal size: 1.41 MB*
badge
* An ASCII character in UTF-8 is 8 bits or 1 byte. |
867caf9 to
bf4974a
Compare
bf4974a to
8237249
Compare
| ".spectrum-Badge--turquoise", | ||
| ".spectrum-Badge--yellow", | ||
| ".spectrum-Badge--subtle", | ||
| ".spectrum-Badge--subtle:where(.spectrum-Badge--accent)", |
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.
FYI in the SWC next-gen web component we updated this class to remove the -style from the class name.
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.
Looks good! Left some questions for building my own knowledge on this work.
| --- | ||
|
|
||
| This update removes `--mod-*` custom property hooks per SWC-1264, see also the RFC for extensible styling. | ||
| This update removes `--mod-*` custom property hooks per SWC-1264, see also the RFC for extensible styling. In addition, this update cleans up any component-level custom properties that did not rely on the CSS cascade to define the styles; this was done to reduce the number of custom properties that are defined at the component level and trim down the size of the CSS we are shipping to consumers. |
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.
Love this update! ❤️
| /* background color variants */ | ||
| &.spectrum-Badge--neutral { | ||
| background: var(--mod-badge-subtle-background-color-default, var(--spectrum-badge-subtle-background-color-default)); | ||
| &:where(.spectrum-Badge--neutral) { |
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.
Tell me more about :where() in this context, it seems like the benefit over trying to match both classes is that it doesn't add specificity, right? I'm especially curious as we carry out similar CSS updates in other components. Will we want to consider this pattern in similar use cases in other components? Any pitfalls to be aware of? Any other things to note? (I believe it was :where in another component in that action menu PR I reviewed where we discovered that we actually did want the added specificity, for instance.)
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.
Yes, great questions. Badge is a good use-case for :where in that there are only a few variants active at any given time. I kept specificity on outline and subtle but where the colors showed up, I let the order they show up in the file be the deciding factor. For more complex components this gets ultra tricky because there are cases where we want that specificity. This seemed like a great opportunity to reduce it though so I added it here.
| /* Style = subtle */ | ||
| .spectrum-Badge--style-subtle { | ||
| color: var(--mod-badge-subtle-label-icon-color, var(--spectrum-badge-subtle-label-icon-color)); | ||
| .spectrum-Badge--subtle { |
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.
I'm on board with this change (and I see it's in barebones already 🎉 ), and given that our changeset will reflect alll the other component changes, I think I can see the rationale for not documenting this change in the changeset and I'm on board with that too... but I still want to highlight it for consideration in case someone else has a strong opinion or we have second 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.
Yes, I did check our changesets to see if there were any references to the new class name and since it wasn't there, I opted to leave it out of the new docs. The concept of what was added is in the previous changeset but not the class name specifically.
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 any plans to carry over these changes to barebones? I'm assuming we'd want this version of the CSS to land in SWC at some point.
Description
This update removes
--mod-badge-*custom property hooks per SWC-1264, see also the RFC for extensible styling. Class selectors and variants remain unchanged; stories were refreshed to match the current API.--mod-badge-*custom property hooks.Breaking change: the
--mod-badge-*override layer is removed. Consumers should set--spectrum-badge-*variables directly where customization as needed.Related issue(s)
Author's checklist
Reviewer's checklist
patch,minor, ormajorfeaturesValidation steps
spectrum-twobaseline.Regression testing
Validate:
The documentation pages for at least two other components are still loading, including:
If components have been modified, VRTs have been run on this branch: