-
Notifications
You must be signed in to change notification settings - Fork 235
chore: update theme docs #5837
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: main
Are you sure you want to change the base?
chore: update theme docs #5837
Conversation
|
📚 Branch Preview🔍 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 |
Tachometer resultsCurrently, no packages are changed by this PR... |
Pull Request Test Coverage Report for Build 19064794047Details
💛 - Coveralls |
2027dbc to
93881db
Compare
2cfaba3 to
fa8657b
Compare
fa8657b to
daf6f70
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.
I left some comments! Looks really good :)
| `sp-theme` applies a Spectrum theme by using CSS custom properties to set default sizes & colors for all of the components in its scope. The Spectrum design system provides four color themes (`lightest`, `light`, `dark`, and `darkest`) and two different scales (`medium` and `large`) to support desktop & mobile UI. | ||
| `<sp-theme>` provides Spectrum design tokens (CSS custom properties) to everything in its DOM scope. Components inside a theme use these tokens to render correctly. The element itself does not visually “apply” styles to your app; it exposes the tokens so your components (and any of your CSS) can consume them. | ||
|
|
||
| Spectrum offers multiple variants: |
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: I see the API tab for Theme is not surfacing the properties / attributes. I recommend looking into it if it is a quick fix.
| Spectrum offers multiple variants: | ||
|
|
||
| - **System**: `spectrum`, `express`, `spectrum-two` | ||
| - **Color**: `light`, `dark` (legacy: `lightest`, `darkest` – deprecated) |
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.
wireframe? it is used below in the "What it does" section
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.
what do you mean by wireframe?
|
|
||
| Spectrum offers multiple variants: | ||
|
|
||
| - **System**: `spectrum`, `express`, `spectrum-two` |
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 to surface the default values?
tools/theme/README.md
Outdated
| ## Advanced usage | ||
|
|
||
| ## System Context (private Beta API - subject to changes) | ||
| ### What it does |
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.
What it does
This heading sounds a bit cryptic... What does it refer to here? And why is this under "Advanced usage"?
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.
you're right... i want to put it at the top in the overview section with title - How sp-theme works
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.
does that sound better?
| import '@spectrum-web-components/theme/src/themes.js'; // spectrum | ||
| import '@spectrum-web-components/theme/src/express/themes.js'; // express | ||
| import '@spectrum-web-components/theme/src/spectrum-two/themes.js'; // spectrum-two |
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: It would be great if you can mention what these imports indicate via a heading rather than a comment?
tools/theme/README.md
Outdated
|
|
||
| updateTheme('light', 'medium'); | ||
| ``` | ||
| ### Light theme, medium scale |
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.
| ### Light theme, medium scale | |
| ### Light color, medium scale |
| ## Dark theme | ||
|
|
||
| ```html demo | ||
| ```html |
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.
| ```html | |
| ```html-no-demo |
tools/theme/README.md
Outdated
| ``` | ||
|
|
||
| ## Large scale | ||
| ### Dark theme, large scale |
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.
| ### Dark theme, large scale | |
| ### Dark color, large scale |
tools/theme/README.md
Outdated
| There are a few cases where it is necessary to embed one theme within another. | ||
| For example, if you have an application that is using a dark theme with a left to right text direction that is | ||
| previewing or editing content that will be displayed in a light theme with a right to left text direction. | ||
| There are a few cases where it is necessary to embed one theme within another. For example, if you have an application that is using a dark theme with a left to right text direction that is previewing or editing content that will be displayed in a light theme with a right to left text direction. |
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 a few cases where it is necessary to embed one theme within another. For example, if you have an application that is using a dark theme with a left to right text direction that is previewing or editing content that will be displayed in a light theme with a right to left text direction. | |
| There are a few cases where it is necessary to embed one theme within another. For example, if you have an application that is using a dark color system with a left to right text direction that is previewing or editing content that will be displayed in a light color system with a right to left text direction. |
tools/theme/README.md
Outdated
| ``` | ||
|
|
||
| ## Embedding themes | ||
| ### Embedded themes and directional content |
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.
| ### Embedded themes and directional content | |
| ### Embedded system and directional content |
tools/theme/README.md
Outdated
| Visually, all Spectrum Web Components are an expression of the design tokens specified by Spectrum, Adobe's design system. On the web, these tokens are made available as CSS Custom Properties. Using `sp-theme` as a parent element for your components ensures that those CSS Custom Properties can be leveraged by the elements within your application. This ensures consistent delivery of not only the color and scale you've specified in your particular instance, but for _all_ the other color, scale, and content direction specifications across Spectrum. | ||
|
|
||
| #### Consuming the System Context in Components | ||
| Additionally, the `sp-theme` element manages the content direction applied to the elements in its DOM scope. By default, an `sp-theme` element resolves its initial content direction from the value specified by its first `sp-theme` or `document` ancestor; otherwise, it uses the value `dir="ltr"` if a content direction isn't present in those elements. When a value for `dir` is manually supplied to `sp-theme`, the default value is overridden. In all cases, though, `sp-theme` manages the `dir` value of its `SpectrumElement` descendents, unless another `sp-theme` element is placed into its scope and overrides that management. | ||
|
|
||
| Components can consume the system context by using the `SystemResolutionController`. This controller encapsulates the logic for resolving the system context, allowing it to be integrated into any component in few steps. | ||
| To make the above concepts a little more concrete, take a look at the table below. Try selecting another `color` in the picker, and notice how that changes the values of the colors. The token names for the variables persist, but the RGB values under the hood change! Considering that `sp-theme` also manages all the other theme and size options, `sp-theme` reveals itself to be a pretty powerful component. |
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.
A visual approach with some code snippet will add more value to this section.
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 is a table that renders below with all color tokens
| ## Accessibility | ||
|
|
||
| - **Color and contrast**: Ensure sufficient contrast for any backgrounds you apply when consuming tokens (WCAG 2.1 AA). Components themselves meet Spectrum guidance when themed correctly. | ||
| - **Language**: Set `lang` on `<sp-theme>` to inform number/date formatting and other locale-aware components. | ||
| - **Directionality**: Use `dir` to deliver correct LTR/RTL behavior; embedded themes allow mixing contexts. |
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.
Not sure if this section falls under a11y!
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's isn't anything else particularly that came to my mind while thinking of A11y stuff for theme and so I added this
|
|
||
| ``` | ||
| ```js | ||
| import '@spectrum-web-components/theme/sp-theme.js'; |
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.
A short comment above this import might be nice (I know the text above the code snippet already kind of explains it, so feel free to disregard!):
| import '@spectrum-web-components/theme/sp-theme.js'; | |
| // Registers the <sp-theme> custom element | |
| import '@spectrum-web-components/theme/sp-theme.js'; |
| </sp-button-group> | ||
| <sp-theme system="spectrum" color="dark" scale="large"> | ||
| <div id="outer"> | ||
| <sp-field-label for="volume">Volume</sp-field-label> |
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.
tools/theme/README.md
Outdated
| ### What it does | ||
|
|
||
| The <sp-theme> element provides a "system" context to its descendants in the DOM. This context indicates the Spectrum design system variant currently in use (e.g., 'spectrum', 'express', or 'spectrum-two'). | ||
| Visually, all Spectrum Web Components are an expression of the design tokens specified by Spectrum, Adobe's design system. On the web, these tokens are made available as CSS Custom Properties. Using `sp-theme` as a parent element for your components ensures that those CSS Custom Properties can be leveraged by the elements within your application. This ensures consistent delivery of not only the color and scale you've specified in your particular instance, but for _all_ the other color, scale, and content direction specifications across Spectrum. |
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 this content from this section in the old version of the documentation? It feels a little more base-level, and I'm wondering if putting it in the "Advanced usage" section might lead to more users ignoring it?
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.
Hmmm. You're right... What do you think about having it in the overview section? Would that be okay?
tools/theme/README.md
Outdated
| </script> | ||
|
|
||
| 2. Instantiate the `SystemResolutionController`: | ||
| When you're ready to look into more advanced usage of the components and themes in your application, there are vanilla CSS implementations of these tokens available in the `@spectrum-web-components/styles` package. |
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.
Linking to the styles package here would be good!

Description
Updated
tools/theme/README.mdto align with documentation standards which include accessibility.Motivation and context
This step will help our consumers use components accessibly. Additionally it gives the accessibility auditors, enough context to audit components.
It will ensure the full API is documented, including each property, slot, method, event, CSS property, and tokens in the component and along with accessibility concerns and/or decisions, linking to accessibility team documentation where applicable.
Related issue(s)
Screenshots (if appropriate)
Author's checklist
I have added automated tests to cover my changes.N/AI have included a well-written changeset if my change needs to be published.N/AReviewer's checklist
Includes thoughtfully written changeset if changes suggested includeN/Apatch,minor, ormajorfeaturesAutomated tests cover all use cases and follow best practices for writingN/AAll VRTs are approved before the author can update Golden HashN/AManual review test cases
Device review