feat: migrate asset to s2 styles and second-gen architecture #5858

TarunAdobe · 2025-11-06T09:01:58Z

Description

This PR migrates the next-gen Asset component to Spectrum 2 styles and updates Storybook accordingly.

Motivation and context

This PR migrates the Asset component from first-gen to the second-gen architecture, following the established pattern of separating core component logic from Spectrum-specific implementation.

Changes:

Migrated CSS to align with Spectrum CSS structure

What changed

First-gen:

first-gen/packages/asset/src/Asset.ts - Remains unchanged for backward compatibility

Core (Base):

second-gen/packages/core/components/asset/Asset.base.ts - Updates shared functionalities
second-gen/packages/core/components/asset/Asset.types.ts - Creates types file for size and static color variants
second-gen/packages/core/components/asset/index.ts - Imports and exports types

Second-gen:

second-gen/packages/swc/components/asset/Asset.ts - Updates render method to accommodate classes
second-gen/packages/swc/components/asset/asset.css - Adds CSS from spectrum-css
second-gen/packages/swc/components/asset/stories/asset.stories.ts - Adds comprehensive story coverage

Related issue(s)

SWC-1261

Screenshots (if appropriate)

Author's checklist

I have read the CONTRIBUTING and PULL_REQUESTS documents.
I have reviewed at the Accessibility Practices for this feature, see: Aria Practices
I have added automated tests to cover my changes.
I have included a well-written changeset if my change needs to be published.
I have included updated documentation if my change required it.

Reviewer's checklist

Includes a Github Issue with appropriate flag or Jira ticket number without a link
Includes thoughtfully written changeset if changes suggested include patch, minor, or major features (Note: I did not add a changeset, I wasn't sure that this was set up for barebones and didn't see any for progress circle or badge, but I'm happy to add one if I'm mistaken!)
Automated tests cover all use cases and follow best practices for writing
Validated on all supported browsers
All VRTs are approved before the author can update Golden Hash

Manual review test cases

Run second-gen Storybook:

yarn workspace @swc/components storybook

Storybook functionality:

All controls work correctly on Default story

Documentation:

All stories render correctly on Docs page
Story descriptions are clear and informative

First-gen verification (use the PR preview or run locally):

yarn start
# or
yarn docs:start

First-gen Asset remains unchanged and functional

Code review checklist:

TypeScript types are correct and properly exported
CSS follows second-gen patterns
No linter errors
Proper separation of concerns between core and SWC packages

Device review

Did it pass in Desktop?
Did it pass in (emulated) Mobile?
Did it pass in (emulated) iPad?

changeset-bot · 2025-11-06T09:02:03Z

⚠️ No Changeset found

Latest commit: 1b8d3f0

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

github-actions · 2025-11-06T09:04:01Z

📚 Branch Preview

🔍 Visual Regression Test Results

When 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: pr-5858

If the changes are expected, update the current_golden_images_cache hash in the circleci config to accept the new images. Instructions are included in that file.
If the changes are unexpected, you can investigate the cause of the differences and update the code accordingly.

package.json

Rajdeepc

Good start!

Rajdeepc · 2025-11-11T06:54:20Z

2nd-gen/packages/swc/components/asset/asset.css

 }

-::slotted(*) {
+.spectrum-Asset-image {


This is a breaking change for the existing behaviour if consumers are slotting an <img> without class.
You can keep this class with a migration shim.

::slotted() { /* same rules */ } .spectrum-Asset-image { /* same rules */ }

but can we not break this for spectrum-two?

we should try not to treat S2 as a chance to introduce breaking changes unless they’re really necessary. These can have a big impact on adoption for product teams, so it’s worth being mindful about when and why we make them. If a change truly adds more value than the potential disruption, we can definitely go for it

2nd-gen/packages/swc/components/asset/asset.css

2nd-gen/packages/swc/components/asset/Asset.ts

Rajdeepc · 2025-11-11T07:01:27Z

2nd-gen/packages/swc/components/asset/asset.css

-.fileBackground {
-    fill: var(--spectrum-asset-file-background);
+.spectrum-Asset-folderBackground {
+    fill: var(--spectrum-gray-200);


Do these map to the same value? Can you surface up these changes in the PR documentation too for quick reference by reviewers. Its hard to visualize from here

These should be the same values, yes (and they look to be mapped correctly).

Before Cassondra left, she started removing some of the complexity from a chunk of components, asset being one of them. You can see the changes (and remove of the --spectrum-asset- variables) here: https://github.com/adobe/spectrum-css/pull/4257/files#diff-3a87a9f21648ac03bf4532993e3cdf48604b9862efd1c0ea68c3f6c2d44b32b3

In a different PR (that does the same and removes mods and some of the --spectrum prefixed variables for asset list and miller), Cassondra left her reasoning as to why: adobe/spectrum-css#4260 (comment)

Hopefully that's useful! I absolutely agree with @Rajdeepc that we should surface the changes in a changeset. None of these migrations have had a changeset yet. (I asked a similar question in Slack when I was working on status light). Do we have the changeset management situation figured out? I've been out, so it might be and we need a changeset!

marissahuysentruyt

I don't think I have any true changes to request! I did leave some additional context in response to a few of Rajdeep's comments, though- hopefully they're helpful!

2nd-gen/packages/swc/components/asset/Asset.ts

2nd-gen/packages/core/components/asset/Asset.base.ts

marissahuysentruyt · 2025-11-11T17:59:17Z

2nd-gen/packages/swc/components/asset/asset.css

-.fileBackground {
-    fill: var(--spectrum-asset-file-background);
+.spectrum-Asset-folderBackground {
+    fill: var(--spectrum-gray-200);


These should be the same values, yes (and they look to be mapped correctly).

Before Cassondra left, she started removing some of the complexity from a chunk of components, asset being one of them. You can see the changes (and remove of the --spectrum-asset- variables) here: https://github.com/adobe/spectrum-css/pull/4257/files#diff-3a87a9f21648ac03bf4532993e3cdf48604b9862efd1c0ea68c3f6c2d44b32b3

In a different PR (that does the same and removes mods and some of the --spectrum prefixed variables for asset list and miller), Cassondra left her reasoning as to why: adobe/spectrum-css#4260 (comment)

Hopefully that's useful! I absolutely agree with @Rajdeepc that we should surface the changes in a changeset. None of these migrations have had a changeset yet. (I asked a similar question in Slack when I was working on status light). Do we have the changeset management situation figured out? I've been out, so it might be and we need a changeset!

2nd-gen/packages/swc/components/asset/asset.css

package.json

marissahuysentruyt · 2025-11-12T18:03:47Z

@TarunAdobe I had one more thing!

Looking at the title of this PR, I'm curious if this work is a feat as opposed to a chore, since it's the migration into second-gen. Should we update this PR title to reflect that? Is my feat vs. chore logic even accurate? Asset is such a small component so I'm not sure if it's really a "breaking" change or not.

marissahuysentruyt

I went through it more in depth, and I have a few more questions for you. It still looks really great!

I'd like 2 changes I think:

I think the big thing for me ended up being in the stories file, just keeping the render method in the meta to align with how the other migrated components have it set up currently.
Pretty sure we need a :host selector in the CSS that also has the display: flex property.

marissahuysentruyt · 2025-11-13T15:38:54Z

2nd-gen/packages/swc/components/asset/asset.css

-        var(--spectrum-asset-icon-outline-color)
-    );
-
+.spectrum-Asset {


The CSS here looks great- matches the S2 code we have in the spectrum-two branch. 👍 🥳

My only question is do we need a selector for :host, giving it the display: flex property? I believe the default display for :host is inline.

Suggested change

.spectrum-Asset {

:host {

display: flex;

}

.spectrum-Asset {

display: flex; // we would also need this here for the `align-items` & `justify-content`

align-items: center;

justify-content: center;

inline-size: 100%;

block-size: 100%;

}

marissahuysentruyt · 2025-11-13T16:33:05Z

2nd-gen/packages/core/components/asset/Asset.base.ts

+import { ASSET_VARIANTS, type AssetVariant } from './Asset.types.js';
+
+/**
+ * @slot - The content to render when no `variant` is provided (typically an <img> element)


There is another JSDoc comment in swc/components/asset/Asset.ts about the @slot that is almost identical to this. Is there a reason to have both? It looks like the content in the swc directory's Asset.ts file is the one being used in the control table when storybook renders.

I'm not opposed to keeping both of them, but I was just curious if we need both, or if some other components should have both.

marissahuysentruyt · 2025-11-13T16:44:20Z

2nd-gen/packages/swc/components/asset/Asset.ts

+ * <swc-asset style="block-size: 128px">
+ *   <img class="spectrum-Asset-image" src="example.png" alt="Example image" />
+ * </swc-asset>
+ *
+ * @example
+ * <swc-asset variant="file" style="min-inline-size: 150px; block-size: 128px"></swc-asset>
+ *
+ * @example
+ * <swc-asset variant="folder" style="min-inline-size: 150px; block-size: 128px"></swc-asset>


For these examples, do we want to encourage setting inline styles with the style attribute? Or is that more often what consumers would do? Maybe we could show just one example with inline styles.

I don't think this is a blocker, but I just noticed each one had inline styles.

marissahuysentruyt · 2025-11-13T17:03:42Z

2nd-gen/packages/swc/components/asset/stories/asset.stories.ts


 export const Default: Story = {
-    render: (args) => html` <swc-asset variant="${args.variant}"></swc-asset> `,
+    render: (args) => template(args),


Just for consistency, I believe the preference Gray was trying to establish is to include this render in the Meta object. So this line would move up under argTypes (line 42).

Then, we can clean up each story by just redefining args:

Suggested change

render: (args) => template(args),

// @ line 42

render: (args) => template(args),

... // the rest of the metadata and the default export...

export const Default: Story = {

args: {

variant: undefined,

},

};

export const File: Story = {

args: {

variant: 'file',

},

tags: ['!dev'],

};

export const Folder: Story = {

args: {

variant: 'folder',

},

tags: ['!dev'],

};

That does however, remove the inline styles you have on the stories, but are those inline styles necessary? Does this so acceptable to you?

TarunAdobe force-pushed the ttomar/asset-migration branch 3 times, most recently from 8500305 to e550a7e Compare November 6, 2025 11:07

TarunAdobe commented Nov 6, 2025

View reviewed changes

package.json Outdated Show resolved Hide resolved

chore: migrate asset

58474d7

TarunAdobe force-pushed the ttomar/asset-migration branch from e550a7e to 58474d7 Compare November 6, 2025 11:13

TarunAdobe marked this pull request as ready for review November 6, 2025 13:54

TarunAdobe requested a review from a team as a code owner November 6, 2025 13:54

Rajdeepc requested changes Nov 11, 2025

View reviewed changes

Rajdeepc added the Status: Ready for review PR ready for review or re-review. label Nov 11, 2025

marissahuysentruyt reviewed Nov 11, 2025

View reviewed changes

TarunAdobe added 2 commits November 13, 2025 16:58

chore: review suggestions

b5ba2d3

chore: merge main:

1b8d3f0

TarunAdobe changed the title ~~chore: migrate asset~~ feat: migrate asset to s2 styles and second-gen architecture Nov 13, 2025

TarunAdobe requested review from Rajdeepc, caseyisonit and marissahuysentruyt November 13, 2025 11:50

marissahuysentruyt requested changes Nov 13, 2025

View reviewed changes

-.spectrum-Asset {
+:host {
+display: flex;
+}
+.spectrum-Asset {
+    display: flex; // we would also need this here for the `align-items` & `justify-content`
+    align-items: center;
+	justify-content: center;
+	inline-size: 100%;
+	block-size: 100%;
+}

-    render: (args) => template(args),
+// @ line 42
+render: (args) => template(args),
+... // the rest of the metadata and the default export...
+export const Default: Story = {
+    args: {
+        variant: undefined,
+    },
+};
+export const File: Story = {
+    args: {
+        variant: 'file',
+    },
+    tags: ['!dev'],
+};
+export const Folder: Story = {
+    args: {
+        variant: 'folder',
+    },
+    tags: ['!dev'],
+};

feat: migrate asset to s2 styles and second-gen architecture #5858

Are you sure you want to change the base?

feat: migrate asset to s2 styles and second-gen architecture #5858

Conversation

TarunAdobe commented Nov 6, 2025 • edited by marissahuysentruyt Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Motivation and context

What changed

Related issue(s)

Screenshots (if appropriate)

Author's checklist

Reviewer's checklist

Manual review test cases

Device review

Uh oh!

changeset-bot bot commented Nov 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

⚠️ No Changeset found

Uh oh!

github-actions bot commented Nov 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

📚 Branch Preview

🔍 Visual Regression Test Results

Uh oh!

Uh oh!

Rajdeepc left a comment

Choose a reason for hiding this comment

Uh oh!

Rajdeepc Nov 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marissahuysentruyt Nov 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marissahuysentruyt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

marissahuysentruyt Nov 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

marissahuysentruyt commented Nov 12, 2025

Uh oh!

marissahuysentruyt left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

TarunAdobe commented Nov 6, 2025 •

edited by marissahuysentruyt

Loading

changeset-bot bot commented Nov 6, 2025 •

edited

Loading

github-actions bot commented Nov 6, 2025 •

edited

Loading

Rajdeepc Nov 11, 2025 •

edited

Loading

marissahuysentruyt Nov 11, 2025 •

edited

Loading

marissahuysentruyt Nov 11, 2025 •

edited

Loading

marissahuysentruyt left a comment •

edited

Loading