docs: Add 2nd-gen TypeScript style guide and Storybook docs automation#6067
docs: Add 2nd-gen TypeScript style guide and Storybook docs automation#6067caseyisonit wants to merge 14 commits intomainfrom
Conversation
|
There was a problem hiding this comment.
this correctly keeps 1st-types working but closer to 1st-gen. this is the pattern for correcting 1st-gen types while maintaining code quality in 2nd-gen.
There was a problem hiding this comment.
Properly reflects the types patterns we are wanting to enforce, the key update with this is NEVER using s2 in any type declarations so we avoid breaking changes in the future to remove that
There was a problem hiding this comment.
implements the as const pattern that will aid in IDE support for allowed values
📚 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 |
There was a problem hiding this comment.
script to auto generate contributor docs to mdx files and work in storybook. CONTRIBUTOR-DOCS remains the source of truth. ties in to yarn start/storybook so it runs when you spin up storybook
There was a problem hiding this comment.
Is this something we want to keep long-term? I feel like this is unnecessary overhead. Could we let Storybook render these files directly and respect the file structure to figure the hierarchy? that would allow us to get rid of the TOC generation, numeric prefix, etc.
| ], | ||
| ], | ||
| 'Contributor docs', | ||
| // GENERATED:CONTRIBUTOR-DOCS-SORT - Do not edit manually. Run `yarn generate:contributor-docs` to update. |
There was a problem hiding this comment.
script generates a story sort based on the directory and file structure in CONTRIBUTOR-DOCS
There was a problem hiding this comment.
Love this! Is my understanding of this correct: if you run yarn storybook or yarn storybook:build, yarn generate:contributor-docs will run automatically? This comment is maybe just a tiny bit misleading in that you probably don't have to go out of your way to update this, and it just happens when you run storybook locally.
But, another thing I think about is:
What if someone makes a docs-only change to CONTRIBUTOR-DOCS, and never runs storybook, so they forget to update this?
Would an AI rule maybe be useful here to help remind? Or some other thing to combat this possible issue?
| 2. **Zero-width non-joiner**: Added `⁠` (zero-width non-joiner) between label text and required asterisk icon to prevent text wrapping issues in internationalized content | ||
| 3. **Static color support**: Added support for `--staticBlack` and `--staticWhite` variants through the `staticColor` parameter | ||
|
|
||
| </details> |
There was a problem hiding this comment.
rogue closing tag that broke rendering
caseyisonit
added
Status:WIP
caseyisonit
added
the
2nd gen
rise-erpelding
left a comment
rise-erpelding
left a comment
There was a problem hiding this comment.
Leaving a few comments, this looks really nice and I do really like having the contributor docs in Storybook as well as GitHub!
| /* | ||
| /** | ||
| * @todo The S1 types can be removed once we are no longer maintaining 1st-gen. | ||
| * @todo Rename STATUSLIGHT_ prefix to STATUS_LIGHT_ to align with type prefix |
There was a problem hiding this comment.
Do we have a ticket to address this somewhere already? I guess we can roll it into something else if not.
There was a problem hiding this comment.
I think these will be addressed as part of the code quality work-streams. cc: @caseyisonit
| } | ||
|
|
||
| /** | ||
| * Rewrite internal .md links to Storybook doc paths. |
There was a problem hiding this comment.
This works beautifully for the vast majority of internal links but leaves out links to GitHub code files like [Badge](../../../2nd-gen/packages/core/components/badge/Badge.base.ts), would it be doable to parse these and link them to the appropriate GitHub files? Or use absolute links either way? Just something to think about, not necessarily a blocker for now but something we should address at some point.
Here are some links for specific examples of what I mean:
- in
02_class-structure.md - in
02_typescript/README.md - in
01_component-css.md(multiple instances in this file) - in
04_making-a-pull-request.md
| ], | ||
| ], | ||
| 'Contributor docs', | ||
| // GENERATED:CONTRIBUTOR-DOCS-SORT - Do not edit manually. Run `yarn generate:contributor-docs` to update. |
There was a problem hiding this comment.
Love this! Is my understanding of this correct: if you run yarn storybook or yarn storybook:build, yarn generate:contributor-docs will run automatically? This comment is maybe just a tiny bit misleading in that you probably don't have to go out of your way to update this, and it just happens when you run storybook locally.
But, another thing I think about is:
What if someone makes a docs-only change to CONTRIBUTOR-DOCS, and never runs storybook, so they forget to update this?
Would an AI rule maybe be useful here to help remind? Or some other thing to combat this possible issue?
| 'Anti patterns', | ||
| 'Property order quick reference', | ||
| ], | ||
| 'Linting tools', |
There was a problem hiding this comment.
I'm also seeing some changes to this file after pulling down the latest changes to the branch and running storybook to regenerate these, do you see that on your end also?
rubencarvalho
left a comment
rubencarvalho
left a comment
There was a problem hiding this comment.
Looks great! Left a couple of small comments. My larger concern is around the Storybook conversion script. Is there a world where we would just write the contributor docs directly in .mdx instead? Happy to defer this to a follow-up dedicated ticket if that makes more sense.
As a general recommendation, I also find these changes easier to review when they’re more scoped - for example, the Storybook generation could have been its own dedicated PR 👍
| }); | ||
| describe('Overlay - type="modal", v1', () => { | ||
| describe('handle multiple separate `contextmenu` events', async () => { | ||
| // @TODO: skipping this test suite because it's flaky in CI. Will review in the migration to Spectrum 2. |
There was a problem hiding this comment.
How did you come to this conclusion? I haven't seen this flake nor does it show on our metrics?
There was a problem hiding this comment.
Also curious why this started flaking here - does not show in any of the recent PRs nor metrics 🕵️
| * @slot - Text label of the badge. | ||
| * @slot icon - Optional icon that appears to the left of the label | ||
| * | ||
| * @todo review the mixin composition here. We currently have 3 levels of mixins on this class, but the mixin composition guide recommends a maximum of 2. |
There was a problem hiding this comment.
I’d challenge the idea that a “maximum of 2” is a reasonable rule. There isn’t really a hard limit we need to follow... though fewer mixins generally makes things easier to reason about and avoids fragile super chains. Where did you find the guidance that 2 is the recommended max?
| /* | ||
| /** | ||
| * @todo The S1 types can be removed once we are no longer maintaining 1st-gen. | ||
| * @todo Rename STATUSLIGHT_ prefix to STATUS_LIGHT_ to align with type prefix |
There was a problem hiding this comment.
I think these will be addressed as part of the code quality work-streams. cc: @caseyisonit
There was a problem hiding this comment.
Is this something we want to keep long-term? I feel like this is unnecessary overhead. Could we let Storybook render these files directly and respect the file structure to figure the hierarchy? that would allow us to get rid of the TOC generation, numeric prefix, etc.
| : html` | ||
| <slot></slot> | ||
| `} | ||
| ${when(this.variant === 'file', () => file(this.label))} |
There was a problem hiding this comment.
This feels like a better use case of the choose directive (reference).
|
|
||
| ## Debug mode API | ||
|
|
||
| The `window.__swc` object provides debug utilities: |
There was a problem hiding this comment.
Could we add a brief explainer of where/when does this get initialized?
| | `ifDefined` | `lit/directives/if-defined.js` | Stories, templates | Skip attribute if undefined | | ||
| | `styleMap` | `lit/directives/style-map.js` | Storybook decorators | Dynamic inline styles | | ||
|
|
||
| ### classMap |
There was a problem hiding this comment.
I am wondering if we should reference the official Lit documentation for these directives? It feels like we may be doing duplicate work here. Lit's examples show the directives signature more clearly too. E.g.:
classMap(classInfo: {[name: string]: string | boolean | number})
nikkimk
left a comment
nikkimk
left a comment
There was a problem hiding this comment.
I'm loving the way this is taking shape. I have a few more suggestions and questions to address before it's good to go, so hit me up when ready for re-review.
| ], | ||
| ], | ||
| 'Contributor docs', | ||
| // GENERATED:CONTRIBUTOR-DOCS-SORT - Do not edit manually. Run `yarn generate:contributor-docs` to update. |
| **Renaming existing merged prefixes:** If a component's types file in core already uses a merged prefix (e.g., `STATUSLIGHT_`), rename the constants to the underscore-separated form (`STATUS_LIGHT_`) in core. To avoid a breaking change in 1st-gen, re-export the old name as a deprecated alias from the 1st-gen package: | ||
|
|
||
| ```typescript | ||
| // 1st-gen/packages/status-light/src/StatusLight.ts | ||
| import { STATUS_LIGHT_VARIANTS_S1 } from '@spectrum-web-components/core/components/status-light'; | ||
|
|
||
| /** | ||
| * @deprecated Use `STATUS_LIGHT_VARIANTS_S1` instead. | ||
| */ | ||
| export const STATUSLIGHT_VARIANTS_S1 = STATUS_LIGHT_VARIANTS_S1; | ||
| ``` | ||
|
|
||
| This keeps the canonical name correct in core while giving 1st-gen consumers a migration path. |
| 1. **Copyright header** | ||
| 2. **Imports** (typically only `ElementSize` from core mixins) | ||
| 3. **Shared constants** — sizes (e.g., `BADGE_VALID_SIZES`), canonical base arrays (e.g., `BADGE_VARIANTS_COLOR`, `BADGE_VARIANTS_SEMANTIC`), non-variant constants (e.g., `FIXED_VALUES`) | ||
| 4. **S1-only constants** — subset arrays exclusive to 1st-gen, validated against canonical types via `satisfies` (e.g., `BADGE_VARIANTS_COLOR_S1`, `BADGE_VARIANTS_S1`) | ||
| 5. **Canonical constants** — composed arrays that spread from shared bases (e.g., `BADGE_VARIANTS`) | ||
| 6. **Types** — shared types (e.g., `BadgeSize`, `BadgeColorVariant`), then S1-only types (e.g., `BadgeVariantS1`), then canonical types (e.g., `BadgeVariant`) |
There was a problem hiding this comment.
<3 the inclusion of examples
| ### Dependency direction | ||
|
|
||
| The dependency always flows toward S1: | ||
|
|
||
| ```text | ||
| Shared constants (base arrays + canonical types) | ||
| ←── Canonical composed arrays spread from shared | ||
| ←── S1 arrays validate against shared types via satisfies | ||
| ``` | ||
|
|
||
| This means: | ||
|
|
||
| - **Canonical base arrays and types live in SHARED** — `BADGE_VARIANTS_COLOR` and `BadgeColorVariant` are defined before S1-ONLY | ||
| - **S1 arrays validate against canonical types** — `BADGE_VARIANTS_COLOR_S1` uses `satisfies readonly BadgeColorVariant[]` | ||
| - **Canonical composed arrays never reference S1 arrays** — `BADGE_VARIANTS` does not spread `BADGE_VARIANTS_S1` | ||
| - **S1 arrays may spread from shared arrays** — `BADGE_VARIANTS_S1` spreads `BADGE_VARIANTS_SEMANTIC` (shared) | ||
| - **Deleting S1 content never breaks shared or canonical content** | ||
|
|
||
| ### Removing S1 | ||
|
|
||
| When 1st-gen is retired, the removal process for each types file is: | ||
|
|
||
| 1. Delete the `S1-ONLY` section (constants and types) | ||
| 2. Delete the `S1-ONLY` section separator | ||
| 3. Remove the `@todo` comment about S1 removal | ||
| 4. Optionally, if a shared constant was only shared for S1's benefit and is identical to the canonical array, inline it | ||
|
|
||
| No renames. No changes to canonical type names. No breaking changes for 2nd-gen consumers. |
|
|
||
| ```text | ||
| Picker → SizedMixin(ExpandableElement) → SpectrumElement | ||
| Combobox → Textfield → ManageHelpText(SizedMixin(Focusable)) |
There was a problem hiding this comment.
| Combobox → Textfield → ManageHelpText(SizedMixin(Focusable)) | |
| Combobox → Textfield → ManageHelpText(SizedMixin(Focusable)) (**Note**: This is very likely to change in 1st-gen in order to fix existing accessilbility issues with comobobox.) |
| | Button / Button Group | Focusable, LikeAnchor, PendingState | Needs Focusable mixin | | ||
| | Action Button / Action Group | Focusable, LikeAnchor, Longpress | Needs Focusable + interaction | | ||
| | Link | Focusable, LikeAnchor | Needs Focusable + LikeAnchor | |
There was a problem hiding this comment.
Links (including buttons and action buttons) are moving to CSS-only typography solutions in 1st-gen. We should be CSS typography alternatives to all buttons in 1st gen.
| | Button / Button Group | Focusable, LikeAnchor, PendingState | Needs Focusable mixin | | |
| | Action Button / Action Group | Focusable, LikeAnchor, Longpress | Needs Focusable + interaction | | |
| | Link | Focusable, LikeAnchor | Needs Focusable + LikeAnchor | | |
| | Button / Button Group | Focusable, LikeAnchor, PendingState | Could be replaced by CSS-only typography solution | | |
| | Action Button / Action Group | Focusable, LikeAnchor, Longpress | Could be replaced by CSS-only typography solution | |
| 5. **Help text controller** — convert `ManageHelpText` mixin to controller pattern | ||
| 6. **Platform utilities** — port `platform.ts` detection functions to `core/utils/` | ||
| 7. **Focus utilities** — port `getActiveElement()`, `firstFocusableIn()`, focusable selectors to `core/utils/` | ||
| 8. **Random ID utility** — port `randomID()` to `core/utils/` for generating unique IDs within shadow roots. Note: IDREFs don't cross shadow boundaries, so this is primarily useful for internal `aria-labelledby`/`aria-describedby` references within the same shadow root, or for non-ARIA purposes like CSS targeting. |
|
|
||
| **Pattern**: Interactive/semantic components (Progress Circle, Divider) set `role` in `firstUpdated()` and manage dynamic ARIA attributes in `updated()`. Non-interactive display components (Badge, Status Light) don't set roles. Asset handles accessibility within its SVG templates. | ||
|
|
||
| **Note**: Divider sets `role="separator"` unconditionally, which is the correct pattern — components should be opinionated about their semantic role. Progress Circle's conditional check is unnecessary complexity that could be simplified to match Divider's approach. |
| 5. **Custom getter/setter for `fixed`** in Badge base has a `@todo` noting it may behave identically to a standard Lit reactive property — unnecessary complexity. | ||
| 6. **Section coverage gaps in tests**: Some components skip standard test categories despite implementing the underlying features. | ||
| 7. **Duplicate JSDoc tags**: Asset SWC has duplicate `@example` annotations. | ||
| 8. **Mixed import styles**: Combining package aliases with deep relative imports increases maintenance overhead. |
There was a problem hiding this comment.
| 8. **Mixed import styles**: Combining package aliases with deep relative imports increases maintenance overhead. | |
| 8. **Mixed import styles**: Combining package aliases with deep relative imports increases maintenance overhead. | |
| 9. **Conditional role assignment**: Conditional ARIA roles in web components are an anti-pattern because each role carries distinct semantic meaning, interaction expectations, and structural rules that cannot be safely swapped at runtime. When a single component toggles between roles — say, switching between listbox and menu — it must also dynamically change which child roles are valid (option vs. menuitem), whether nesting is permitted, how keyboard navigation behaves, and what events assistive technologies expect. This complexity is inherently fragile: a missed edge case means the component silently violates the ARIA spec, producing an experience that appears accessible but fails in practice. A clearer approach is to split components along role boundaries. For example, sp-menu and sp-listbox should be separate components because a menu permits menuitem, menuitemcheckbox, and menuitemradio children and supports nested submenus, while a listbox permits only option children and has no concept of nesting. Merging these into one component obscures those constraints and forces the implementation to reconcile two incompatible interaction models. The same principle applies to swatch and swatch-group: a swatch that is purely informational (no role or role="img"), one that acts as a toggle button (role="button" with aria-pressed), one that acts as a checkbox (role="checkbox" with aria-checked), and one that acts as a radio button (role="radio" within a radiogroup) each have fundamentally different semantics, keyboard behaviors, and parent-container requirements. Collapsing all of these into a single component with a conditional role shifts the burden of correctness onto the consumer, who must know exactly which configuration produces a valid accessibility tree — the opposite of what a component library should do. Splitting components by role makes the choice explicit, enforces valid structure through the API itself, and ensures that each component can be tested and documented against a single, well-defined ARIA contract. |
| ### Priority 5: Standardize accessibility patterns | ||
|
|
||
| 1. **Add accessible name validation** to Badge base class (for icon-only variants). Status Light does not require accessible name validation — consumers are responsible for including text alongside the component. | ||
| 2. **Use unconditional role assignment** everywhere. Components should be opinionated about their semantic role. Simplify Progress Circle to match Divider's pattern. |
There was a problem hiding this comment.
Progress Circle should always be a progressbar. Pending state should use an animated icon, since it does not communicate actual progress.
4 participants
Description
Adds a comprehensive TypeScript style guide for 2nd-gen component development and automates publishing contributor docs to Storybook.
Motivation and context
Contributors (human and AI) need clear, consistent guidance for writing 2nd-gen components. This PR provides detailed conventions for file organization, class structure, decorators, types, and composition patterns—all based on the Badge component as the reference implementation.
Additionally, contributor docs are now automatically converted and displayed within Storybook, making them easier to discover and browse alongside component documentation.
Related issue(s)
Screenshots (if appropriate)
N/A - documentation changes
Author's checklist
Reviewer's checklist
patch,minor, ormajorfeaturesManual review test cases
Verify TypeScript guide renders correctly in Storybook
yarn storybookin2nd-gen/packages/swcVerify navigation structure is accurate
CONTRIBUTOR-DOCS/02_style-guide/02_typescript/README.mdDevice review
Summary of changes:
generate-contributor-docs.mjs