docs(storybook): docs cleanup#6495
Conversation
|
rise-erpelding
added
Status:WIP
📚 Branch Preview Links🔍 Gen1 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 |
Coverage Report for CI Build 29448418525Warning No base build found for commit Coverage: 96.257%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsRequires a base build to compare against. How to fix this → Coverage Stats
💛 - Coveralls |
810e7a5 to
2827051
Compare
rise-erpelding
added
Status:Ready for review
There was a problem hiding this comment.
This migration hit main right when the effort to switch stories to mdx did, I believe, so it is the only component that doesn't have an mdx page right now. Converting this is in the list of follow-up tickets!
Rajdeepc
added
the
High priority PR review
| ### Em dashes | ||
|
|
||
| Do not use em dashes (`—`) in documentation prose or code comments. Replace them with the punctuation that matches the sentence's intent: | ||
|
|
||
| | Em dash usage | Replace with | | ||
| | ------------------------------------- | ----------------------------- | | ||
| | Clause separator | Semicolon (`;`) | | ||
| | Introducing elaboration or an example | Colon (`:`) or a new sentence | | ||
| | Parenthetical aside | Comma or parentheses | | ||
|
|
||
| ```markdown | ||
| <!-- ❌ Bad --> | ||
|
|
||
| appearance — disabled colors and an animated spinner | ||
| fill-only — fill-style="outline" is not supported | ||
|
|
||
| <!-- ✅ Good --> | ||
|
|
||
| appearance: disabled colors and an animated spinner | ||
| fill-only; fill-style="outline" is not supported | ||
| ``` |
There was a problem hiding this comment.
The Adobe Spectrum writing guidelines don't actually have any problem with em dashes, but do explicitly say not to use semicolons. This is a bit contradictory to what we have here.
I'm working on getting some clarification here, but have been hesitant to try to do a full pass of the docs to remove or scale back em dashes if the agent is going to replace many of them with semicolons, which Adobe finds objectionable.
For now it is being added to my list of tickets to file in the PR description.
There was a problem hiding this comment.
Guidance regarding em dashes: we can certainly continue to try to scale back on these because they're overused in AI.
Guidance regarding semicolons: we'll want to limit them, but I don't think we have to ban them. They shouldn't be the default substitute for em dashes though, and we should definitely avoid using any one punctuation pattern.
3ccf175 to
1a788ce
Compare
najikahalsema
left a comment
najikahalsema
left a comment
There was a problem hiding this comment.
Submitting my review before my next meeting--I'm still reviewing the a11y docs!
najikahalsema
left a comment
najikahalsema
left a comment
There was a problem hiding this comment.
Submitting my review before my next meeting--I'm still reviewing the a11y docs!
najikahalsema
left a comment
najikahalsema
left a comment
There was a problem hiding this comment.
Reviewed the rest of the files :)
najikahalsema
left a comment
najikahalsema
left a comment
There was a problem hiding this comment.
Reviewed the Core overview and a11y guides in this pass. Will follow up w/ another review of customisation/ and resources/ files.
TLDR: RIP semicolons :( my favourite punctuation mark :(
najikahalsema
left a comment
najikahalsema
left a comment
There was a problem hiding this comment.
Will do "learn-about-swc" and "resources" dirs tomorrow :^)
I need to follow up with Content strat about the "right" way to refer to our components in the case of stuff like "Button styling". 🫡
Not finished yet! But GitHub is slowing way down and I can't apply others right now. Co-authored-by: Najika Halsema Yoo <44980010+najikahalsema@users.noreply.github.com> Co-authored-by: rise-erpelding <54716846+rise-erpelding@users.noreply.github.com>
Co-authored-by: Najika Halsema Yoo <44980010+najikahalsema@users.noreply.github.com> Co-authored-by: rise-erpelding <54716846+rise-erpelding@users.noreply.github.com>
Co-authored-by: Najika Halsema Yoo <44980010+najikahalsema@users.noreply.github.com> Co-authored-by: rise-erpelding <54716846+rise-erpelding@users.noreply.github.com>
48c2368 to
badde08
Compare
| - **Hold affordance** (`hold-affordance` / `longpress`): not yet available. In the interim, separate the secondary action into a distinct `swc-action-button` within an action group, which is more accessible than relying on a longpress interaction alone. | ||
| - **Toggle button**: selection and toggle UX is planned via `swc-toggle-button` and `swc-toggle-button-group`, available in a future release. | ||
|
|
||
| <DocsFooter /> |
There was a problem hiding this comment.
Noting that our API tables don't parse the descriptions properly, for instance:
I will make a follow up ticket for this.
I also think the text in our tables seems a bit small, @nikkimk is this an accessibility issue? Does it need to be at least 16px (it's 12.25px with code being 10.5px)? Would it be worthwhile to have a ticket to make the font size bigger?
43fad9e to
5c93559
Compare
rubencarvalho
added
skip_vrt
najikahalsema
left a comment
najikahalsema
left a comment
There was a problem hiding this comment.
Okay, all done with the content pass! Looking good!
| Even when a section has only one story (e.g. a single `States` story called `States`), the `### Title` subheading appears under the `## Section` heading. This matches the `SpectrumStories` block's behavior when `hideTitle=false`. | ||
| When a section has only one story, and that story's rendered name is identical to the `## Section` heading (e.g. a single `States` story named `States`), omit the `### Title` subheading. Author the prose directly under `## Section`, with the `<Canvas>` reference immediately below it. A `## States` heading followed immediately by a `### States` subheading is redundant; collapse it to one heading. (`button.mdx`, `color-handle.mdx`.) | ||
|
|
||
| If a single-story section's rendered name _differs_ from the `## Section` heading (e.g. a `## Behaviors` section whose only story renders as "Toggle behavior"), keep the `### Title` subheading. It's adding real information there, not repeating the section title. |
There was a problem hiding this comment.
| If a single-story section's rendered name _differs_ from the `## Section` heading (e.g. a `## Behaviors` section whose only story renders as "Toggle behavior"), keep the `### Title` subheading. It's adding real information there, not repeating the section title. | |
| If a section of a single story has a rendered name that _differs_ from the `## Section` heading, (e.g., a `## Behaviors` section whose only story renders as "Toggle behavior"), keep the `### Title` subheading. This adds valuable context because it's not simply repeating the section title. |
| 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 | ||
| 1. Use of style-related attributes per component such as `variant` and `size` |
There was a problem hiding this comment.
| 1. Use of style-related attributes per component such as `variant` and `size` | |
| 1. Use style-related attributes per component, such as `variant` and `size` |
There was a problem hiding this comment.
Using "simple" verb tenses instead of -ing ending
| 5. extending the components to include custom styles | ||
| 6. [globally overriding design token](#globally-override-design-tokens) custom properties | ||
| 1. Use of style-related attributes per component such as `variant` and `size` | ||
| 2. Providing [overrides of component custom properties](#overriding-component-custom-properties) |
There was a problem hiding this comment.
| 2. Providing [overrides of component custom properties](#overriding-component-custom-properties) | |
| 2. Provide [overrides of component custom properties](#overriding-component-custom-properties) |
| 6. [globally overriding design token](#globally-override-design-tokens) custom properties | ||
| 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 |
There was a problem hiding this comment.
| 3. _Coming soon:_ writing styles for specific component shadow parts | |
| 3. _Coming soon:_ Write styles for specific component shadow parts |
| 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 |
There was a problem hiding this comment.
| 4. _Coming soon:_ providing granular customization via the theme object | |
| 4. _Coming soon:_ Provide granular customization via the theme object |
| ## Step 7 — Decommission Gen1 | ||
| ## Step 7: Decommission Gen1 | ||
|
|
||
| Once no `<sp-*>` element remains in your app: |
There was a problem hiding this comment.
| Once no `<sp-*>` element remains in your app: | |
| Once there are no more `<sp-*>` elements in your app: |
|
|
||
| ## What's not migrated yet | ||
|
|
||
| Gen2 is shipping components incrementally. The index of components on our docs site lists all components with a Gen2 implementation; anything not listed is still Gen1 only. Plan to keep the Gen1 package installed alongside Gen2 until your components of interest are migrated. |
There was a problem hiding this comment.
| Gen2 ships components incrementally. The index of components on our docs site lists all components with a Gen2 implementation. Anything not listed remains in Gen1 only. Plan to keep the Gen1 package installed alongside Gen2 until your components of interest are migrated. |
| Running Gen1 and Gen2 together is expected during migration, but avoid keeping both versions of the same component on a page longer than necessary. This helps prevent shipping extra component code and loading unnecessary CSS custom properties. | ||
|
|
||
| - Remove the corresponding `@spectrum-web-components/*` per-element import as soon as the last `<sp-*>` usage of that component is gone, so each migrated component drops out of the bundle. | ||
| - Migrate one component at a time, by route or feature area if possible, so the dual-import window for any given page is short. |
There was a problem hiding this comment.
| - Migrate one component at a time, by route or feature area if possible, to shorten the dual-import window for any given page. |
| ## Get help | ||
|
|
||
| - Each component documents its own breaking changes in `<Component>/Migration guide`. | ||
| - The [customization guides](/docs/guides-customization-getting-started--docs) cover styling, theming, and fonts in depth. |
There was a problem hiding this comment.
| - The [customization guides](/docs/guides-customization-getting-started--docs) cover styling, theming, and fonts in-depth. |
|
|
||
| - Each component documents its own breaking changes in `<Component>/Migration guide`. | ||
| - The [customization guides](/docs/guides-customization-getting-started--docs) cover styling, theming, and fonts in depth. | ||
| - [Open an issue on GitHub](https://github.com/adobe/spectrum-web-components/issues/new/choose) for migration questions or gaps. |
There was a problem hiding this comment.
| - [Open an issue on GitHub](https://github.com/adobe/spectrum-web-components/issues/new/choose) if you have any questions about the migration process or would like to report a bug. |
najikahalsema
left a comment
najikahalsema
left a comment
There was a problem hiding this comment.
Okay, all done with the content pass! Looking good!
5t3ph
left a comment
5t3ph
left a comment
There was a problem hiding this comment.
Great job on this, appreciate your detailed eye!
General comment: some of the bolding seems sporadic, perhaps we need a formatting rule around that. Maybe start with removing it inside of sentences and allowing it for like bullet subheaders? Could be a deferred ticket!
Open to any of these being deferred tickets.
| - **Fill**: colored ring segment showing current progress | ||
| - **Label**: accessible text describing the operation, provided through the `label` attribute or property (not visually rendered) | ||
|
|
||
| > **A11y Note**: Light DOM children are not projected into the shadow tree, so content between the opening and closing tags does not supply an accessible name. Use the `label` attribute or property, or `aria-label` / `aria-labelledby` on the host. |
There was a problem hiding this comment.
Suggest removing unless we have a codifiable way to determine when/what a11y rules get promoted here vs. stay in the Accessibility section.
Edit: coming back to say it seems we do have a pattern of noting labeling guidance here, but simplified to a sentence and not in a blockquote, example in Button.
| - **`white`**: use on dark color or image backgrounds | ||
| - **`black`**: use on light color or image backgrounds | ||
|
|
||
| Each panel lists **small**, **medium**, and **large** in order so Chromatic can snapshot every static-color and size pairing with the same label text (stroke weight is what changes). |
There was a problem hiding this comment.
Remove any text like this throughout component docs that is an implementation detail / belongs better in a code comment or our Contributor Docs or skills.
|
|
||
| ### In Action Button | ||
|
|
||
| An avatar can be placed inside an action button to create a user-triggered action tied to a specific person or entity. |
There was a problem hiding this comment.
I'm not sure where this guidance comes from - perhaps we remove and file a ticket to support avatar in action button once we validate if this is meant to be supported? I don't see it in the new Figma for either component.
|
|
||
| ### Best practices | ||
|
|
||
| - Always set `alt`: omitting it causes some screen readers to announce the image URL |
There was a problem hiding this comment.
Can you reconcile this with the "Alt text" section prior? Also the demonstrated snippet doesn't use the decorative attribute as instructed. Looking at the implementation, only decorative is actually needed, not the empty alt, because it is supposed to then set aria-hidden which is functionally equivalent, so other references to an empty alt should probably update to prioritize recommending setting decorative only.
There's also a bug with the Playground controls where decorative doesn't get applied.
|
|
||
| The `level` attribute sets the heading level (`h2`–`h6`) applied to every item header. The default is `3`; values outside the range are clamped. | ||
|
|
||
| > **Note**: In Spectrum 1, `level` could be set on each `sp-accordion-item`; in Spectrum 2 it is set only on `<swc-accordion>`. See the [migration guide](?path=/docs/components-accordion-migration-guide--docs). |
There was a problem hiding this comment.
Another type of note in component docs to remove throughout (if any more exist) is anything referencing S1, these belong in the migration guides only.
|
|
||
| Badges come in four sizes to fit various contexts: | ||
|
|
||
| - **Small (`s`)**: default size; compact spaces, inline with text, or in tables |
There was a problem hiding this comment.
I'm quite sure this "guidance" is hallucinated :D We were not very strict about catching these types of hallucinations in the early batches, but most often sizes are to match parent/relative components and not due to visual hierarchy as is implied here. We have kept these simplified withn a sentence with no guidance in other places (example: Button) or listed notable visual differences (example: Illustrated Message).
Maybe another deferred ticket is evaluating the actual UI/UX guidance presented in especially the intros, sizes, and variants.
| - No keyboard interaction is required or expected | ||
|
|
||
| > **Important**: In focus mode, only interactive elements and their associated labels/descriptions are announced. If content is not a label or description for a focusable element, it will not be read. For non-interactive content, screen reader users must [switch to Browse mode](https://swcpreviews.z13.web.core.windows.net/pr-6122/docs/second-gen-storybook/?path=/docs/guides-accessibility-guides-screen-reader-testing--readme#screen-reader-modes). This is expected behavior, not a bug: ensure you test both modes when evaluating component accessibility. | ||
| > ⚠️ **Important:** In focus mode, only interactive elements and their associated labels/descriptions are announced. If content is not a label or description for a focusable element, it will not be read. For non-interactive content, screen reader users must [switch to browse mode](/docs/guides-accessibility-guides-screen-reader-testing--docs#screen-reader-modes). This is expected behavior, not a bug: ensure you test both modes when evaluating component accessibility. |
There was a problem hiding this comment.
This note seems a bit out of place in the sense it is general a11y information, and why is it important to status light specifically? For example, it would also apply to something like Badge. Tagging @nikkimk to weigh in.
I would also suggest we avoid using blockquote syntax and emojis for things like this, and have a ticket to create maybe some custom doc blocks (or prioritize inline alert 👀 ) to be able to add styled callouts (would also replace the inline style rendered callouts in consumer migration guides).
There was a problem hiding this comment.
@5t3ph I had noted that the migration guides use a hardcoded inline alert (under Styling), it makes sense that we should use the same thing in other places as well, that didn't make it into my list of tickets on the PR but I will add it.
Determining whether it's worthwhile to actually have a component migrated for this would be really nice though, so we don't have to change hardcoded styles later.
There was a problem hiding this comment.
@5t3ph @rise-erpelding in the a11y migration docs of each non focusable component (badge, status-light, etc), I recommended including a specific callout because we often get support messages from people who don't know that non-focusable elements won't be read by the screen reader in focus mode, so they assume these components aren't accessible.
This could be a doc block that we pull in to multiple component docs, but I really do think having that block as a note in each non-focusable component is useful in each component because they users are less likely to find it if it's not in the a11y section of the component docs.
As it is, that note does link to a more detailed explanation our a11y guides.
7 participants
Description
.ai/rules/text-formatting.md, codifying capitalization,<kbd>usage, CSS-class-in-prose, em dash, backtick-value, UI-element-naming, and callout conventions for docs going forward. Updated.ai/rules/stories-documentation.mdand.ai/rules/stories-format.mdto match the structural decisions below (Anatomy shape, static-color story pattern, single-story-section headings, narrowed Behaviors "Focus management" guidance).## Anatomysection now uses a flat, unordered list of parts with slot names called out inline, replacing five inconsistent shapes (numbered lists,### Content/#### Slots/#### Propertiessplits,### Visual structure/### Technical structuresplits). Applied to accordion, tabs, badge, avatar, status-light, color-loupe, color-handle, progress-circle, meter, divider, and illustrated-message.static-colorprose in action-button, button, progress-circle, meter, divider, and link to a single sourced template ("Usestatic-colorwhen the component needs to sit on top of a photo or colored background..."), sourced from Spectrum's published static-color usage guidance. Removed the unused three-story static-color pattern (StaticBlack/StaticWhite/StaticColors) from the stories rules in favor of the combined pattern every component already uses.Disabledstate out of## Optionsinto a new## Statessection (with a matching story retag); deduplicated divider's orientation content, which was documented twice (once in Options, once in Behaviors); consolidated thePendingstate fully into## Statesfor both action-button and button, removing the duplicate## Behaviorsentry each had.scripts/validate-docs-pages.js(yarn lint:docs-pages), currently scoped to component/internal genres.storyNameoverride exists; audited and added missing overrides across accordion, link, tooltip, tabs, badge, status-light, typography, icon, opacity-checkerboard, and progress-circle so every hand-authored heading matches what actually renders.@adobe/spectrum-wc/components/color-loupe→@adobe/spectrum-wc/components/color-loupe/swc-color-loupe.js, the path that actually registers the element).@csspropentries toButton.ts(2),IllustratedMessage.ts(10),Tab.ts(4), andTabs.ts(1), and missing@firesentries toTooltip.base.ts(4), so the generated API tables match the component CSS/events. Fixed an invalid@exampleinBadge.ts(fixed="fill"is not a valid value; corrected tofixed="block-end"). Removed a stale "Breaking change" note fromTabs.base.ts'sdirectionproperty doc that was rendering inside the live API table. Documented accordion'sfocus()/click()method delegation and corrected an avataroutline-default claim that belonged under Avatar Group, not the single-avatar Outline section. All source-file changes are JSDoc-comment-only; no runtime behavior changed.preview.ts; removed a stale "Action bar" entry with no corresponding page; sentence-cased the Customization submenu labels to match their renamed pages. Deduplicatedgetting-started.mdx's installation instructions againstget-started.mdx. Fixed a copy/paste bug intheme-scales.mdx("theme" where "scale" was meant). Added a missing H1 toscreen_reader_testing.mdx. Fixed broken/imprecise anchor links inaccessibility_resources.mdxand an inaccurate cross-reference inaccessible_pattern_libraries.mdx. Clarified inbuild-with-ai.mdxthat there is not yet a Gen2-specific Agent Skill orllms.txt.<kbd>; standardized> **Important**:blockquotes to⚠️ **Important:**; backticked bare class/attribute-value names in bulleted lists; bolded UI element names on first mention; converted bare WCAG success-criterion mentions into links to the W3C Understanding docs. Note: not all em dashes were addressed, see the PR comment to learn why.section-orderstory parameter from all 10 stories inaccordion.stories.ts(section order is hand-authored in MDX, not story-driven). Collapsed a redundant## States/### Statesheading pair incolor-handle.mdxinto one heading. Removed two stray JSDoc comments duplicating MDX content fromtypography.stories.ts. Removed a duplicated "Rendering and styling migration analysis" sidebar entry mistakenly copy-pasted into the Toast section.Follow-up tickets to be filed (11)
typography.mdxis the only component doc with no Accessibility section at all. Real, sourceable content exists (link underline styling, semantic-tag-to-type-style mapping, heading-order best practice); needs a new MDX section plus a new accessibility-tagged story, since neither exists yet.fixedattribute validation: thefixedattribute has no runtime validation or dev-mode warning for invalid values, unlike the siblingvariantattribute. An invalid value silently produces a dead CSS class with no visible effect and no error.swc-tabandswc-tab-panelhave their own CSS custom properties and slots that never render on the tabs docs page because the story config only exposes the parentswc-tabstable. A related undocumented custom property on the tab panel also needs annotating.roleattribute that the main docs page already documents correctly.Motivation and context
Continuation of a documentation-quality pass on the 2nd-gen Storybook docs site: an initial style-guide sweep (capitalization,
<kbd>usage, em dashes) expanded into a deeper structural-consistency pass across every migrated component's docs page (Anatomy, Options, States, Behaviors), plus fixes for verified inaccuracies and gaps surfaced during review (invalid examples, missing API annotations, broken links, an incorrect migration-guide import path). No component runtime behavior changed.Related issue(s)
Screenshots
N/A, documentation only.
Author's checklist
Reviewer's checklist
patch,minor, ormajorfeaturesManual review test cases
Component doc pages read consistently
/docs/components-accordion--docs,/docs/components-badge--docs, and/docs/components-progress-circle--docs## Anatomysection is a flat bullet list with slot names called out inline, no### Contentor#### Slots/#### Propertiessubsections### Static colorssubsection (where present) reads with the same "Usestatic-colorwhen..." opening sentenceAvatar/divider/button Options-States-Behaviors placement
/docs/components-avatar--docs; confirmDisabledappears under## States, not## Options/docs/components-divider--docs; confirm orientation is documented once, under## Optionsonly/docs/components-button--docsand/docs/components-action-button--docs; confirmPendingappears once, under## States, with no separate## BehaviorsentryLint check passes
yarn lint:docs-pagesMigration guides render correctly
/docs/components-accordion-migration-guide--docsand/docs/components-color-loupe-migration-guide--docs@adobe/spectrum-wc/components/color-loupe/swc-color-loupe.jsAPI tables reflect new annotations
/docs/components-button--docs,/docs/components-illustrated-message--docs, and/docs/components-tooltip--docs--swc-button-max-inline-size,--swc-button-down-state-transform, the illustrated-message illustration/typography properties, and the Events table lists all four tooltip open/close eventsSidebar navigation is correct
Device review
Accessibility testing checklist