feat(action-button): adds migration plan for the S2 action button component#6327
feat(action-button): adds migration plan for the S2 action button component#6327cdransf wants to merge 1 commit into
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
changed the title
cdransf
force-pushed
the
cdransf/s2-action-button-migration-plan
branch
from
a2058bf to
a2b079b
Compare
cdransf
added
Status:Ready for review
5t3ph
left a comment
5t3ph
left a comment
There was a problem hiding this comment.
This is half of my review, will pick up again tomorrow :)
|
|
||
| The 1st-gen implementation uses `--spectrum-actionbutton-*` and `--mod-actionbutton-*` token chains via imported `action-button.css`, `spectrum-action-button.css`, and `action-button-overrides.css`. The full modifier surface includes: | ||
|
|
||
| **Sizing and spacing:** |
There was a problem hiding this comment.
[nit] no need to list all of these :)
|
|
||
| --- | ||
|
|
||
| ## React Spectrum S2 API surface |
There was a problem hiding this comment.
[nit] I think we can skip this section as long as it was used as a reference in planning to determine the SWC API.
|
|
||
| | # | What changes | 1st-gen behavior | 2nd-gen behavior | Consumer migration path | | ||
| |---|---|---|---|---| | ||
| | **B1** | Remove `href` / link API | `href` and related attributes cause `sp-action-button` to proxy a hidden anchor. A dev warning was added in 1st-gen. | `swc-action-button` is button-only. Navigation uses native `<a>` elements. | Replace `<sp-action-button href="...">` with a styled native `<a>` element. | |
There was a problem hiding this comment.
Due to the link behavior removal, we need to plan to also output global styles and show examples on the related global elements docs page. With Button, I setup a Vite plugin to automatically generate those from the primary stylesheet, see the README for vite-global-elements-css.
5t3ph
left a comment
5t3ph
left a comment
There was a problem hiding this comment.
A few suggestions on the API
React also supports Avatar and Badge, where Avatar + Badge or Icon + Badge has a different lockup. We should probably mark those as Additive and create tickets. Both those affect the accessible name so we should have @nikkimk review those conditions specifically in the follow-ups.
| | `size` | `'xs' \| 's' \| 'm' \| 'l' \| 'xl'` | `'m'` | `size` | **Confirmed.** Includes `xs` — differs from `swc-button` which starts at `s`. Requires `ACTION_BUTTON_VALID_SIZES` in `ActionButton.types.ts`. No default attribute (`noDefaultSize: true`): `getAttribute('size')` returns `null` until a consumer sets it explicitly; the JS property defaults to `'m'` but is not reflected to the DOM automatically. | | ||
| | `quiet` | `boolean` | `false` | `quiet` | **Confirmed.** Retained as a primary visual differentiator (no background/border at rest). Unlike Button's deprecated `quiet`, this is a first-class visual treatment for action-button. | | ||
| | `staticColor` | `'white' \| 'black' \| undefined` | `undefined` | `static-color` | **Confirmed.** Static color for use over images or colored backgrounds. Supported with both default and `quiet` treatments. | | ||
| | `value` | `string` | `''` | `value` | **Confirmed.** Retained for identification within action groups. JS property defaults to `''`; when the stored value is empty (`''`), the component reads `textContent` as the effective value for action-group identification. Consumers who rely on the fallback must not also set `value=""` and expect textContent to win. | |
There was a problem hiding this comment.
React ActionButtonGroup does not offer the "selected" functionality, so we will probably be dropping that for Gen2. Keeping it also mixes our ARIA concerns which we're trying to avoid. I'd say drop or at least defer this attribute. Even if we keep it, maybe doing like we did for tabs and changing it to like action-button-id rather than value to avoid a forms connotation.
| | `staticColor` | `'white' \| 'black' \| undefined` | `undefined` | `static-color` | **Confirmed.** Static color for use over images or colored backgrounds. Supported with both default and `quiet` treatments. | | ||
| | `value` | `string` | `''` | `value` | **Confirmed.** Retained for identification within action groups. JS property defaults to `''`; when the stored value is empty (`''`), the component reads `textContent` as the effective value for action-group identification. Consumers who rely on the fallback must not also set `value=""` and expect textContent to win. | | ||
| | `disabled` | `boolean` | `false` | `disabled` | **Confirmed.** Inherited from `ButtonBase`. Maps to native `disabled` on the internal `<button>`. | | ||
| | `pending` | `boolean` | `false` | `pending` | **Additive (A1).** New in 2nd-gen. Follows the exact same contract as `swc-button`. Button remains focusable; activation is suppressed. | |
There was a problem hiding this comment.
This doesn't need to be additive since it has partial functionality from extending button base and can copy from button to finish it out.
| | `disabled` | `boolean` | `false` | `disabled` | **Confirmed.** Inherited from `ButtonBase`. Maps to native `disabled` on the internal `<button>`. | | ||
| | `pending` | `boolean` | `false` | `pending` | **Additive (A1).** New in 2nd-gen. Follows the exact same contract as `swc-button`. Button remains focusable; activation is suppressed. | | ||
| | `accessibleLabel` | `string \| undefined` | `undefined` | `accessible-label` | **Confirmed.** Replaces 1st-gen `label`. Forwarded as `aria-label` on the internal `<button>`. Required for icon-only usage. Inherited from `ButtonBase`. | | ||
| | `pendingLabel` | `string \| undefined` | `undefined` | `pending-label` | **Additive (A1).** New in 2nd-gen. Custom accessible label during pending state. When omitted, derived from resolved name + `", busy"` suffix. Inherited from `ButtonBase`. | |
There was a problem hiding this comment.
Also not additive, goes along with pending.
| | `role` | removed | n/a | removed | **Confirmed removal.** `swc-action-button` is always `role="button"`. No consumer-controlled role override. | | ||
| | `href`, `target`, `download`, `referrerpolicy`, `rel` | removed | n/a | removed | **Confirmed removal.** Navigation uses native anchors. | | ||
| | `type` | deferred beyond initial scope | `'button'` | deferred | **Deferred.** Initial 2nd-gen scope defaults to `button` only; `submit` / `reset` are future work matching `swc-button`. | | ||
| | `active` | internal | n/a | internal | **Confirmed internal.** Retained as an internal property inherited from `ButtonBase`. With hold-affordance deferred, no consumer-triggered mechanism sets this in initial scope. CSS `:active` on the inner `<button>` handles pressed-state styling without requiring a JS property. | |
There was a problem hiding this comment.
Probably just remove entirely?
| | `autofocus` | `boolean` | `false` | `autofocus` | **Confirmed — inherited.** With `delegatesFocus: true`, `autofocus` on the host routes initial focus to the inner `<button>` automatically. This differs from 1st-gen where the host itself was the focus target; consumers who check `document.activeElement` after autofocus must expect the inner `<button>`. | | ||
| | `tabIndex` | `number` | managed | `tabindex` | **Confirmed — inherited.** Setting `tabIndex=-1` on the host removes the component from the tab order; used by `swc-action-group` for roving tabindex management. With `delegatesFocus: true`, positive `tabIndex` routes tab focus through the host to the inner `<button>`. | |
There was a problem hiding this comment.
Do we need to explicitly include these since they are native DOM attributes? We don't for Button.
2 participants
Description
Adds the Phase 1 migration plan for the
swc-action-button1st-gen → 2nd-gen migration atCONTRIBUTOR-DOCS/03_project-planning/03_components/action-button/migration-plan.md.The plan covers:
@spectrum-web-components/action-button@1.12.1) with all properties, events, slots, CSS custom properties, and shadow DOM structurependingstateautofocus,tabIndex,name, andactiveswc-action-buttonextendsButtonBasedirectly; no separateActionButton.base.tsin core is neededswc-action-groupKey decisions:
toggles,selected,emphasized, and consumer-controlledroleare removed; toggle UX moves toswc-toggle-button/swc-toggle-button-grouphold-affordance/longpressare deferred with documented consumer optionspendingis a new additive feature matching theswc-buttoncontract inButtonBaseaccessible-label(attribute) replaceslabelsizeincludesxs, requiringACTION_BUTTON_VALID_SIZESin a newActionButton.types.tshrefand the full link API are removedMotivation and context
Phase 1 of the washing machine migration workflow for
action-button. The plan is the shared review surface all subsequent migration phases (Setup through Review) depend on before any implementation begins. It supersedes ad-hoc planning in comments and consolidates decisions from the action-button accessibility migration analysis and rendering and styling migration analysis into a single actionable document.Related issue(s)
Author's checklist
Reviewer's checklist
Manual review test cases
This PR contains only planning documentation. No runtime behavior changes; no UI to test.
Plan is internally consistent — API decisions table accounts for every 1st-gen property
CONTRIBUTOR-DOCS/03_project-planning/03_components/action-button/migration-plan.mdBreaking changes align with source material
toggles/selectedremoval), B4 (hold-affordancedeferral), and B6 (roleremoval) match the analysis conclusions