feat(close-button): migrate to 2nd-gen#6410
Conversation
🦋 Changeset detectedLatest commit: 6a20470 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
TarunAdobe
changed the title
TarunAdobe
force-pushed
the
ttomar/close-button-migration
branch
from
4b96756 to
49359cc
Compare
📚 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 |
nikkimk
left a comment
nikkimk
left a comment
There was a problem hiding this comment.
In addition to the linting error, I think we should look at removing the default slot. The default slot is used for visual labels and there is never a visible label (only an accessible one) on the close button.
| ${crossIconBySize[this.size]()} | ||
| </span> | ||
| <span class="swc-CloseButton-label"> | ||
| <slot></slot> |
There was a problem hiding this comment.
Since the slotted label is never visible, and we already have accessible-label, we should probably just remove the slotted label, and update docs stories and tests accordingly.
| <slot></slot> |
There was a problem hiding this comment.
I’ve removed the slot and label span from the render tree, dropped the .swc-CloseButton-label styles, and updated getResolvedAccessibleName() so accessible-label is the only naming channel (light DOM text is ignored). Stories, docs, migration guide, unit tests, and Playwright a11y snapshots are updated accordingly.
Also fixed the lint issues.
There was a problem hiding this comment.
The default slot is still showing up on the API page.
pfulton
added
Status:Ready for review
TarunAdobe
force-pushed
the
ttomar/close-button-migration
branch
from
6f61eb9 to
1b96e38
Compare
5t3ph
left a comment
5t3ph
left a comment
There was a problem hiding this comment.
Please ask the agent to run the code conformance rule prior to these changes - it will hopefully catch some of the things I noted for CSS, let me know if it doesn't at least do the rule re-ordering for you!
| * decorative (`aria-hidden="true"`). | ||
| * | ||
| * @element swc-close-button | ||
| * @since 0.0.1 |
There was a problem hiding this comment.
| * @since 0.0.1 | |
| * @since 2.0.0 |
| - **Large (`l`)** — larger hit target when space allows | ||
| - **Extra large (`xl`)** — maximum visibility for prominent dismiss controls | ||
|
|
||
| > **Migration note:** `<swc-close-button>` reflects **`size="m"`** on the host when omitted. Spectrum 1 did not reflect a default `size` attribute, though the visual default was still medium. |
There was a problem hiding this comment.
Remove all migration notes - any of this type of information belongs in the consumer migration guide only.
| box-sizing: border-box; | ||
| } | ||
|
|
||
| :host([size="s"]) { |
There was a problem hiding this comment.
[nit] re-order these t-shirt sizes and other variants to the end of the file per our rule order
| transition-property: outline, border-color, color, background-color, transform; | ||
| } | ||
|
|
||
| .swc-CloseButton:focus { |
| --swc-close-button-icon-color-hover: token("transparent-black-900"); | ||
| } | ||
|
|
||
| .swc-CloseButton { |
There was a problem hiding this comment.
There is a transition error for the outline color on :focus-visible (previously mitigated with action button as well) that is resolved by splitting up the outline properties in this way (keep your tokens, simplified for example):
button {
transition: outline-color 300ms ease-in;
outline-offset: 2px;
outline: 0px solid transparent;
}
button:focus-visible {
outline-width: 2px;
outline-color: blue;
}There was a problem hiding this comment.
One additional recommendation I might make for outline styling, to align more with button-base.css, if you set a transparent border or outline it will automatically become visible when in forced-colors/WHCM mode, even when not focused:
To avoid that, button sets outlines only for :focus-visible, like this:
.swc-Button {
/* no outline styles here */
}
.swc-Button:focus-visible {
outline: token("focus-indicator-thickness") solid var(--swc-button-focus-indicator-color, token("focus-indicator-color"));
outline-offset: token("focus-indicator-gap");
}I think we could implement that here, unless there is a reason that we would want the double outline.
There was a problem hiding this comment.
@5t3ph explained the outline error to me more and I'd recommend a tiny adjustment to avoid issues with transition (see SWC-2308):
.swc-CloseButton {
outline: outline: 0px solid transparent;
}| outline-offset: var(--swc-close-button-focus-indicator-gap, token("focus-indicator-gap")); | ||
| } | ||
|
|
||
| .swc-CloseButton:hover:not(:disabled) { |
There was a problem hiding this comment.
This increases the specificity more than we'd like (same for :active).
Instead, borrow the pattern from button-base.css and remove the inflation here to exclude disabled and replace the current :disabled selector with .swc-CloseButton:disabled:is(*, :hover)
| block-size: 100%; | ||
| } | ||
|
|
||
| .swc-CloseButton-icon svg path { |
There was a problem hiding this comment.
[nit] this should be inherited if placed on svg - if it is not, there may be a fill property within the icon SVG that shouldn't be there.
|
Reminder to add the |
| * @slot - Visible button label. | ||
| * @slot icon - Optional leading icon. |
There was a problem hiding this comment.
Since base does not include the render, slot JS docs should probably move to the component classes. Not all buttons will have a default slot (close button, clear button, and infield button do not have it) and now all buttons have an icon slot, and it shows up in close button's API.
| * @slot - Visible button label. | |
| * @slot icon - Optional leading icon. |
TarunAdobe
force-pushed
the
ttomar/close-button-migration
branch
from
9991067 to
d0a30da
Compare
rise-erpelding
left a comment
rise-erpelding
left a comment
There was a problem hiding this comment.
This will also need a changeset!
| --swc-close-button-icon-color-hover: token("transparent-black-900"); | ||
| } | ||
|
|
||
| .swc-CloseButton { |
There was a problem hiding this comment.
One additional recommendation I might make for outline styling, to align more with button-base.css, if you set a transparent border or outline it will automatically become visible when in forced-colors/WHCM mode, even when not focused:
To avoid that, button sets outlines only for :focus-visible, like this:
.swc-Button {
/* no outline styles here */
}
.swc-Button:focus-visible {
outline: token("focus-indicator-thickness") solid var(--swc-button-focus-indicator-color, token("focus-indicator-color"));
outline-offset: token("focus-indicator-gap");
}I think we could implement that here, unless there is a reason that we would want the double outline.
TarunAdobe
force-pushed
the
ttomar/close-button-migration
branch
from
d0a30da to
0d8fdab
Compare
|
Thank you @rise-erpelding for a thorough review. I have addressed these:
|
Coverage Report for CI Build 28227431428Coverage increased (+0.007%) to 96.246%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
TarunAdobe
force-pushed
the
ttomar/close-button-migration
branch
from
16df1ab to
4347e04
Compare
| user-select: none; | ||
| transition-timing-function: token("animation-ease-in-out"); | ||
| transition-duration: token("animation-duration-100"); | ||
| transition-property: border-color, color, background-color, transform; |
There was a problem hiding this comment.
| transition-property: border-color, color, background-color, transform; | |
| transition-property: outline, border-color, color, background-color, transform; |
| --swc-close-button-icon-color-hover: token("transparent-black-900"); | ||
| } | ||
|
|
||
| .swc-CloseButton { |
There was a problem hiding this comment.
@5t3ph explained the outline error to me more and I'd recommend a tiny adjustment to avoid issues with transition (see SWC-2308):
.swc-CloseButton {
outline: outline: 0px solid transparent;
}| - [ ] Lint/tests/build pass. | ||
| - [ ] Status doc updated. | ||
| - [ ] Changeset included. | ||
| - [x] Status doc updated. |
There was a problem hiding this comment.
Claude has informed me that we still need to update 01_status.md
There was a problem hiding this comment.
your claude is smarter than my claude lol
TarunAdobe
force-pushed
the
ttomar/close-button-migration
branch
from
3d51ec1 to
48b9a83
Compare
5t3ph
left a comment
5t3ph
left a comment
There was a problem hiding this comment.
A few last documentation suggestions.
| ### Properties | ||
|
|
||
| - **`accessible-label`** — required accessible name forwarded to the inner button as `aria-label` | ||
| - **`size`** — visual size of the hit target and icon scale (`s`, `m`, `l`, `xl`) | ||
| - **`static-color`** — static color treatment for display over colored or image backgrounds (`white`, `black`) |
There was a problem hiding this comment.
Remove - these are documented through the other story examples plus the API table
| ### Properties | |
| - **`accessible-label`** — required accessible name forwarded to the inner button as `aria-label` | |
| - **`size`** — visual size of the hit target and icon scale (`s`, `m`, `l`, `xl`) | |
| - **`static-color`** — static color treatment for display over colored or image backgrounds (`white`, `black`) |
| - **Small (`s`)** — compact chrome such as dense toolbars or inline banners | ||
| - **Medium (`m`)** — default size for dialogs, toasts, and most dismiss affordances | ||
| - **Large (`l`)** — larger hit target when space allows | ||
| - **Extra large (`xl`)** — maximum visibility for prominent dismiss controls |
There was a problem hiding this comment.
Are these from our design guidelines, or invented by your agent? 😄
There was a problem hiding this comment.
100% hallucinated but looked okay so i let it be... I'll remove this extra guidance lol
| - **white** — use on dark or colored backgrounds for better contrast | ||
| - **black** — use on light backgrounds for better contrast | ||
|
|
||
| The canvas below uses the Storybook static-color demo layout: **white** on a dark gradient panel and **black** on a light gradient panel. |
There was a problem hiding this comment.
Implementation detail, remove.
| The canvas below uses the Storybook static-color demo layout: **white** on a dark gradient panel and **black** on a light gradient panel. |
TarunAdobe
force-pushed
the
ttomar/close-button-migration
branch
from
a6eca21 to
c20992c
Compare
TarunAdobe
force-pushed
the
ttomar/close-button-migration
branch
from
c20992c to
1b53a3b
Compare
Rajdeepc
changed the title
5t3ph
left a comment
5t3ph
left a comment
There was a problem hiding this comment.
A few last questions on the updates to Button base
| * @internal | ||
| */ | ||
| protected getResolvedAccessibleName(): string | null { | ||
| public getResolvedAccessibleName(): string | null { |
There was a problem hiding this comment.
It looks like you still ended up writing a custom function, so does this still need to be made public?
| */ | ||
| protected readonly handleClick = (event: Event): void => { | ||
| if (this.disabled || this.pending) { | ||
| protected handleActivationClick(event: Event): void { |
There was a problem hiding this comment.
What was the reasoning for this extra distinction of handleActivationClick vs. handeClick? Was this used in an earlier iteration then changed?
Also, why was the || this.pending clause removed/added as an else if? Pending does also need to prevent additional interaction.
| * Static color treatment for display over colored or image backgrounds. | ||
| */ | ||
| @property({ type: String, reflect: true, attribute: 'static-color' }) | ||
| public staticColor?: ButtonStaticColor; |
There was a problem hiding this comment.
Clear and In-field buttons will not need static colors, so are we sure we want to move this to base?
- S3: clarify that standalone use retains its own focus-visible ring; the outline:none suppression applies only in composed field contexts - Drop 'active' from public API: not in ButtonBase; :active CSS handles pressed visual; no toggle use case in-field - Drop 'type' from public API: not in ButtonBase; concrete template hardcodes type="button"; submit/reset deferred to A3 - static-color: add note that if ButtonBase gains staticColor (PR #6410), swc-infield-button must explicitly exclude it - Default slot: fix wording — slots are in the concrete template, not provided by ButtonBase (which has no render method) - Render template: replace BEM modifier classMap with attribute selectors; quiet and size are reflected so :host([quiet]) / :host([size="s"]) apply - Remove fill wrapper div from template; add Phase 5 note to verify whether the wrapper is required or ::slotted() alone is sufficient Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- S3: clarify that standalone use retains its own focus-visible ring; the outline:none suppression applies only in composed field contexts - Drop 'active' from public API: not in ButtonBase; :active CSS handles pressed visual; no toggle use case in-field - Drop 'type' from public API: not in ButtonBase; concrete template hardcodes type="button"; submit/reset deferred to A3 - static-color: add note that if ButtonBase gains staticColor (PR #6410), swc-infield-button must explicitly exclude it - Default slot: fix wording — slots are in the concrete template, not provided by ButtonBase (which has no render method) - Render template: replace BEM modifier classMap with attribute selectors; quiet and size are reflected so :host([quiet]) / :host([size="s"]) apply - Remove fill wrapper div from template; add Phase 5 note to verify whether the wrapper is required or ::slotted() alone is sufficient Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
* chore(close-button): add Cross400 and Cross500 icons Add cross icon assets required for close-button l and xl sizes. Co-authored-by: Cursor <cursoragent@cursor.com> * feat(close-button): phase 2 swc scaffold Add swc-close-button with minimal render, CSS, Storybook stories, and tests for the phase 2 setup milestone. Co-authored-by: Cursor <cursoragent@cursor.com> * refactor(button): move static-color to ButtonBase and harden disabled clicks Co-authored-by: Cursor <cursoragent@cursor.com> --------- Co-authored-by: Cursor <cursoragent@cursor.com>
* feat(close-button): phase 3–4 API stories, tests, and migration plan Rebase onto close-button-migration after scaffold merge. Close-button extends ButtonBase directly (no CloseButtonBase); hide inherited pending controls in Storybook. Co-authored-by: Cursor <cursoragent@cursor.com> * refactor(close-button): use BUTTON_* constants from core in stories Drop redundant CloseButton.VALID_SIZES and STATIC_COLORS aliases; close-button shares the button size and static-color scale via ButtonBase. Co-authored-by: Cursor <cursoragent@cursor.com> * chore(close-button): stop hiding pending controls in Storybook Inherited ButtonBase pending attributes are fine until pending moves off ButtonBase; update migration plan to match. Co-authored-by: Cursor <cursoragent@cursor.com> * test(close-button): address API PR review feedback Use template() for the slot-label Accessibility story example and add a render-time test that accessible-label wins over slot text. Co-authored-by: Cursor <cursoragent@cursor.com> --------- Co-authored-by: Cursor <cursoragent@cursor.com>
* feat(close-button): migrate S2 styling and focus treatment Port spectrum-two close-button visuals to close-button.css with size and static-color token overrides, interactive states, pseudo-element focus ring, and forced-colors support. Document exposed --swc-close-button-* properties and add an Anatomy story for Phase 5 verification. Co-authored-by: Cursor <cursoragent@cursor.com> * docs(close-button): add Storybook docs and consumer migration guide Flesh out close-button.mdx with anatomy, options, states, and accessibility sections. Add migration-guide.mdx for sp-close-button upgrades and align stories with Phase 7 docs conventions. Co-authored-by: Cursor <cursoragent@cursor.com> * chore(close-button): drop unused static modifier classes and align plan Static-color styling is driven by :host([static-color]) in CSS. Update the migration plan TL;DR, sequencing, and Q1 to reflect ButtonBase-only architecture and CSS-only icon scale. Co-authored-by: Cursor <cursoragent@cursor.com> * fix(close-button): align focus and press treatment with button patterns Adopt border/outline focus ring, press perspective transform, and remove custom forced-colors overrides per review. Drop redundant default variant from the static colors story and update docs/tests. Co-authored-by: Cursor <cursoragent@cursor.com> --------- Co-authored-by: Cursor <cursoragent@cursor.com>
…ge (#6405) Complete Phase 6 testing with ARIA snapshots for key stories and tab-focus checks for delegatesFocus and disabled skip behavior. Co-authored-by: Cursor <cursoragent@cursor.com>
Re-add the mixin composition @todo that was dropped when static-color documentation was added during close-button migration. Co-authored-by: Cursor <cursoragent@cursor.com>
Close buttons are icon-only dismiss controls with no visible label. Enforce accessible-label as the sole naming channel and update stories, tests, and docs per review feedback. Co-authored-by: Cursor <cursoragent@cursor.com>
Remove redundant properties text, simplify size descriptions, and drop static-colors implementation detail. Co-authored-by: Cursor <cursoragent@cursor.com>
Revert close-button-driven changes from ButtonBase/Button and keep static-color behavior on CloseButton so this migration is scoped and independent from shared action-button/base coordination. Co-authored-by: Cursor <cursoragent@cursor.com>
TarunAdobe
force-pushed
the
ttomar/close-button-migration
branch
from
308eb53 to
d3a314d
Compare
Replace deprecated close-button token references with current token names so SWC build and targeted component tests pass. Co-authored-by: Cursor <cursoragent@cursor.com>
5t3ph
left a comment
5t3ph
left a comment
There was a problem hiding this comment.
Noticed this was a nuanced case where the intended icon token matches were supposed to be to workflow icon sizes.
| } | ||
|
|
||
| :host([size="s"]) { | ||
| --swc-close-button-icon-size: token("ui-icon-large"); |
There was a problem hiding this comment.
| --swc-close-button-icon-size: token("ui-icon-large"); | |
| --swc-close-button-icon-size: token("ui-icon-small"); |
| } | ||
|
|
||
| :host([size="l"]) { | ||
| --swc-close-button-icon-size: token("ui-icon-2x-large"); |
There was a problem hiding this comment.
| --swc-close-button-icon-size: token("ui-icon-2x-large"); | |
| --swc-close-button-icon-size: token("ui-icon-large"); |
| } | ||
|
|
||
| :host([size="xl"]) { | ||
| --swc-close-button-icon-size: token("ui-icon-2x-large"); |
There was a problem hiding this comment.
| --swc-close-button-icon-size: token("ui-icon-2x-large"); | |
| --swc-close-button-icon-size: token("ui-icon-extra-large"); |
| inline-size: var(--swc-close-button-icon-size, token("ui-icon-extra-large")); | ||
| block-size: var(--swc-close-button-icon-size, token("ui-icon-extra-large")); |
There was a problem hiding this comment.
Not a typo, the default Medium and also Large are both spec'd to be the same large size.
| inline-size: var(--swc-close-button-icon-size, token("ui-icon-extra-large")); | |
| block-size: var(--swc-close-button-icon-size, token("ui-icon-extra-large")); | |
| inline-size: var(--swc-close-button-icon-size, token("ui-icon-large")); | |
| block-size: var(--swc-close-button-icon-size, token("ui-icon-large")); |
7 participants
Description
Completes the 2nd-gen migration of Close Button (Epic SWC-2087) by landing the full stacked workstream
on
main:ButtonBasereuse, cross icons, exportsSWC (
2nd-gen/packages/swc/components/close-button/)CloseButton— extendsButtonBase; native inner<button type="button">with delegated focus;cross icon per
size(s|m|l|xl)size,static-color,accessible-label,disabledclose-button.css— Spectrum 2 close-button visuals (size/static-color states, border + outlinefocus, press
perspectivetransform, aligned with 2nd-gen button interaction patterns)Close Button/Testsinteraction suiteclose-button.mdx,migration-guide.mdxclose-button.a11y.spec.ts(ARIA snapshots + tab-focus)Core / shared
ButtonBase—static-coloron shared button base; DEBUG validation for invalidstatic-colorandmissing
accessible-labelon icon-only controls (inherited by close-button)Cross400Icon,Cross500Iconforl/xlsizesBreaking changes (1st-gen → 2nd-gen)
<sp-close-button>→<swc-close-button>label→accessible-label(or default slot text)variant="white|black"not supported — usestatic-coloronly--mod-closebutton-*→ reviewed--swc-close-button-*sp-close-buttonin 1st-gen is unchanged in this PR.Motivation and context
Close button is a compact dismiss affordance used across dialogs, toasts, banners, and action bars. This PR delivers the full washing-machine migration: API contract, S2 visual parity, accessibility behavior, automated test coverage, and consumer-facing migration docs — bringing
swc-close-buttonto the same standard as other migrated 2nd-gen components.Related issue(s)
Screenshots (if appropriate)
2nd-gen Storybook → Close Button → Docs — verify Anatomy, Sizes, Static colors, States, and Accessibility. Also review Close Button / Migration guide.
Author's checklist
CONTRIBUTING and
PULL_REQUESTS documents.
@adobe/spectrum-wcpatch changeset before merge.)Reviewer's checklist
patch,minor, ormajorfeaturesManual review test cases
correct.
size,static-color,accessible-label,disabled.variantonswc-close-button.accessible-label→ inneraria-label.accessible-labelprecedence.accessible-labelon icon-only instance warns in dev.yarn workspace @adobe/spectrum-wc test -- --run --project storybook components/close-buttonyarn workspace @adobe/spectrum-wc playwright test components/close-button/test/close-button.a11y.spec.ts - project=chromiumaccessible-label, droppingvariant, and--swc-close-button-*custom properties.Device review
Accessibility testing checklist
:focus-visible.accessible-label="Close"— announced as button, name "Close".aria-hidden="true"on icon wrapper).Notes for reviewers
pending/pending-labelin API table — inherited fromButtonBase; close-button does not implement pending visuals. Expected to clear when pending moves offButtonBase(separate button workstream).01_status.md— update Close Button row on merge if not included in this PR.