-
Notifications
You must be signed in to change notification settings - Fork 239
(5/5) Feature: update prefix to swc, complete docs
#5944
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: seckles/feature/swc-vscode-token
Are you sure you want to change the base?
(5/5) Feature: update prefix to swc, complete docs
#5944
Conversation
|
📚 Branch Preview Links🔍 First Generation Visual Regression Test ResultsWhen a visual regression test fails (or has previously failed while working on this branch), its results can be found in the following URLs:
Deployed to Azure Blob Storage: If the changes are expected, update the |
cdransf
left a comment
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.
✨
nikkimk
left a comment
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.
Very informative and easy to understand. I love it when I learn things from a PR.
| 1. use of style related attributes per component such as `variant` and `size` | ||
| 2. providing [overrides of component custom properties](#overriding-component-custom-properties) | ||
| 3. _coming soon:_ writing styles for specific component shadow parts | ||
| 4. _coming soon:_ providing granular customization via the theme object | ||
| 5. extending the components to include custom styles | ||
| 6. [globally overriding design token](#globally-override-design-tokens) custom properties |
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 love that these are in sequence of simplest to complex to encourage consumers to try the simplest, often most robust choice.
| } | ||
| ``` | ||
|
|
||
| If your override is applied too broadly, re-assess how the selector is scoped. |
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.
nit: Can we provide an example and explain the impact?
| ## Rule Order | ||
|
|
||
| Follow this outline for ordering rulesets within component stylesheets. This will provide consistency across component stylesheets, as well as help mitigate common specificity issues. | ||
|
|
||
| 1. *If applicable*: `@layer` , `@keyframes` | ||
| 2. `:host` | ||
| - Considered as a "container" for the component, and should not directly manage component styles itself. Its only concern is how the web component participates in the layout. | ||
| - Primarily includes a `display` property to match the layout flow intent, using either `inline-block` or `block` | ||
| - Avoid `inline` which would prevent consumers applying reasonable layout properties such as `margin` | ||
| - May also include "defensive" styles for `inline-block` elements | ||
| - `place-self: start` to avoid stretch behaviors if they are placed in a flex or grid context | ||
| - `vertical-align: middle` to keep a vertically centered position next to other `inline-block` elements (ex. a row of badges outside of a flex or grid context) | ||
| 3. `* { box-sizing: border-box; }` | ||
| - Unless there is a strong reason not to, this rule should be included in all components. Expand to pseudo-elements if in use. | ||
| - Required if the component or its descendants set `padding` and/or `border` to avoid the compounding affect against the element's size. | ||
| 4. component styles | ||
| - base class: `.swc-ComponentName` | ||
| - subcomponents: `.swc-ComponentName-sub-component` | ||
| - t-shirt sizes: `:host([size="s"])` | ||
| - Uses `:host()` to maintain exposure of size-related custom properties | ||
| - other variants | ||
| - `.swc-ComponentName--variant` - used for variants excluded from custom property exposure | ||
| - `:host([variant="value"])` - used for variants that should maintain custom property exposure | ||
| - states: `:host([aria-expanded])` , `:host:focus-visible` , etc. | ||
| - states should be attached to `:host/:host()` unless the WC expresses the state on internal elements | ||
| - include within variant styles if needed for specificity reasons | ||
| - `::slotted()` and `slot` styles | ||
| - `@media (forced-colors: active)` | ||
| - **First ensure that the styles are actually necessary**. Do not include styles if the browser defaults achieve visibility of critical component parts and states. | ||
| - If needed, this media query should *always* be at the end of the stylesheet to take priority over other styles. | ||
| - Use direct internal selectors, not host selectors, to enforce the style and prevent accidental consumer overrides. |
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.
<3 this section. Very helpful and easy to follow.
|
|
||
| - `:host` is for defining how the container participates in the global layout, not the core component styles | ||
| - Follow the prescribed rule order | ||
| - Strive to keep selector specificity ≤ `\(0,1,0\)` |
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.
nit: Can we link to an anchor to the Managing Specificity section here?
| To keep specificity low, clauses beyond a single class can be wrapped in `:where()` which nulls the specificity of that clause to zero. Use of `:where()` is encouraged, has ample browser support, and will be expected in PR reviews for compounding class selectors. | ||
|
|
||
| ```css | ||
| /* Before */ | ||
| .swc-Divider--staticWhite.swc-Divider--sizeL | ||
|
|
||
| /* After */ | ||
| .swc-Divider--staticWhite:where(.swc-Divider--sizeL) | ||
| ``` |
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 love learning new things via PR reviews!
|
|
||
| **Exceptions to max-specificity rule**: | ||
|
|
||
| - use of pseudo-classes and pseudo-elements are an acceptable bump to specificity, and should be applied outside of `:where()` |
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.
nit: Add inline examples here?
rise-erpelding
left a comment
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.
Leaving some comments, I'm planning to come back to this to digest it a bit more later though! I'd also love to see some of this documentation presented/discussed at a future sync meeting, since it is very thorough but very important!
Overall, this is amazing work and will be such a huge improvement to our system, how we write CSS, and the consistency of the styles we ship!
| 5. extending the components to include custom styles | ||
| 6. [globally overriding design token](#globally-override-design-tokens) custom properties | ||
|
|
||
| SWC strongly recommends using the least intrusive method possible for assigning custom styles. This will help keep your component styles overall inline with Spectrum design recommendations, and reduce the liklihood of compatibilty issues as the library evolves. |
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.
[nit] a couple of small typos I noticed!
| SWC strongly recommends using the least intrusive method possible for assigning custom styles. This will help keep your component styles overall inline with Spectrum design recommendations, and reduce the liklihood of compatibilty issues as the library evolves. | |
| SWC strongly recommends using the least intrusive method possible for assigning custom styles. This will help keep your component styles overall inline with Spectrum design recommendations, and reduce the likelihood of compatibility issues as the library evolves. |
| - `vertical-align: middle` to keep a vertically centered position next to other `inline-block` elements (ex. a row of badges outside of a flex or grid context) | ||
| 3. `* { box-sizing: border-box; }` | ||
| - Unless there is a strong reason not to, this rule should be included in all components. Expand to pseudo-elements if in use. | ||
| - Required if the component or its descendants set `padding` and/or `border` to avoid the compounding affect against the element's size. |
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.
| - Required if the component or its descendants set `padding` and/or `border` to avoid the compounding affect against the element's size. | |
| - Required if the component or its descendants set `padding` and/or `border` to avoid the compounding effect against the element's size. |
|
|
||
| Often, specs will be very prescriptive about what equates to `padding` for an element. That padding may vary for scenarios such as between the top of the component to it's text label vs. from the top to an optional icon. | ||
|
|
||
| It is tempting to use those values as prescribed in order to match the specs. However, the `display` choice of `grid` or `flex` should be taken into account first. |
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 think there's a double space after display here that seems like it could be a typo.
| It is tempting to use those values as prescribed in order to match the specs. However, the `display` choice of `grid` or `flex` should be taken into account first. | |
| It is tempting to use those values as prescribed in order to match the specs. However, the `display` choice of `grid` or `flex` should be taken into account first. |
| - Prefer `gap` over overly prescriptive selectors that apply or remove margin | ||
| - *Exception*: if using `grid-template-areas` , the `gap` will still exist even if the grid area is not populated, so `margin` may be more appropriate | ||
| - Prefer alignment properties in coordination with min/max sizes before overly prescribing `padding` values | ||
| - *Example*: for Badge, there is a `min-block-size` and the specs provide different block padding values for an icon vs. a text label. By using flexbox and `align-items: center` , we only really need to set the *text-relative* block padding, which is more of a defense mechanism in case the badge label needs to wrap to prevent the text touching the component edge. |
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.
There are also spaces between align-items: center and the comma following it.
| - *Example*: for Badge, there is a `min-block-size` and the specs provide different block padding values for an icon vs. a text label. By using flexbox and `align-items: center` , we only really need to set the *text-relative* block padding, which is more of a defense mechanism in case the badge label needs to wrap to prevent the text touching the component edge. | |
| - *Example*: for Badge, there is a `min-block-size` and the specs provide different block padding values for an icon vs. a text label. By using flexbox and `align-items: center`, we only really need to set the *text-relative* block padding, which is more of a defense mechanism in case the badge label needs to wrap to prevent the text touching the component edge. |
| For example: | ||
|
|
||
| - Prefer `gap` over overly prescriptive selectors that apply or remove margin | ||
| - *Exception*: if using `grid-template-areas` , the `gap` will still exist even if the grid area is not populated, so `margin` may be more appropriate |
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.
Do we want to remove this space between grid-template-areas and the comma?
| - *Exception*: if using `grid-template-areas` , the `gap` will still exist even if the grid area is not populated, so `margin` may be more appropriate | |
| - *Exception*: if using `grid-template-areas`, the `gap` will still exist even if the grid area is not populated, so `margin` may be more appropriate |
| - This distinction directly affects which selector type is used (`:host()` vs internal class selectors). See [Variant Selectors and Inheritance](01_component-css.md#shadow-dom-specificity-and-custom-property-inheritance). | ||
| - May be exposed via inclusion in private property, or inline with CSS property | ||
| - Include in private property if value has repeated usage throughout base (non-variant) component styles | ||
| - In migrated components, legacy `--mod-* `properties should not be preserved; instead, collapse the chain into a single component-level property. |
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.
| - In migrated components, legacy `--mod-* `properties should not be preserved; instead, collapse the chain into a single component-level property. | |
| - In migrated components, legacy `--mod-*` properties should not be preserved; instead, collapse the chain into a single component-level property. |
|
|
||
| ## Adding Global Tokens | ||
|
|
||
| Additional global tokens or token overrides may be necessary if values are unique to SWC, and not available - currently or planned - in the design token source package, `@adobe/spectrum-tokens` . |
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.
| Additional global tokens or token overrides may be necessary if values are unique to SWC, and not available - currently or planned - in the design token source package, `@adobe/spectrum-tokens` . | |
| Additional global tokens or token overrides may be necessary if values are unique to SWC, and not available - currently or planned - in the design token source package, `@adobe/spectrum-tokens`. |
|
|
||
| <!-- Document content (editable) --> | ||
|
|
||
| Use this checklist when opening or reviewing a PR. |
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 👏 AMAZING 👏 , what a great idea to include a checklist!
|
|
||
| ## Contributor TL;DR | ||
|
|
||
| > For examples of all these rules in practice, review the [reference migration for Badge](#reference-migration-badge) as well as examples for avoiding issues in the [anti-patterns guide](05_anti-patterns.md). |
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.
Would this link for Badge reference migration work better?
| > For examples of all these rules in practice, review the [reference migration for Badge](#reference-migration-badge) as well as examples for avoiding issues in the [anti-patterns guide](05_anti-patterns.md). | |
| > For examples of all these rules in practice, review the [reference migration for Badge](04_spectrum-swc-migration.md#reference-migration-badge) as well as examples for avoiding issues in the [anti-patterns guide](05_anti-patterns.md). |
|
|
||
| ## Variants & States | ||
|
|
||
| - [ ] Variants that should expose custom properties use `:host([variant="…"])` |
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.
Was the use of " intentional here? GitHub Markdown doesn’t unescape entities in backticks, so it renders literally.
Description
This is PR number 5 of 5
This PR has two main bodies of work:
swc(most of the file changes)Motivation and context
Author's checklist
Reviewer's checklist
patch,minor, ormajorfeaturesManual review test cases
Please thoroughly read the docs to make sure they are understandable and useful, both for ourselves and contributors.