feat(asset): exploring 2nd gen usage#6021
Conversation
|
aramos-adobe
changed the title
📚 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 |
Rajdeepc
left a comment
Rajdeepc
left a comment
There was a problem hiding this comment.
Good effort. Noticed a few structural gaps in this PR.
The image-related types are duplicated in both Asset.types.ts and Image.types.ts, which is a drift — these should be consolidated into a shared source or imported from one place.
The new swc-image component is also introduced without any unit or accessibility tests (there’s no components/image/test/ coverage), which we shouldn’t ship for a new component.
The accessibility documentation in asset.stories.ts is quite thin. It’s missing keyboard behavior (even stating non-interactive), visual accessibility considerations, ARIA implementation details, and clarification of error-state semantics.
I’ve left more inline comments, but these should be addressed before moving forward.
| @property({ type: String }) | ||
| public src?: string; | ||
|
|
||
| /** | ||
| * Alternative text for the image. Required for accessibility when using `src`. | ||
| */ | ||
| @property({ type: String }) | ||
| public alt?: string; | ||
|
|
||
| /** | ||
| * Loading behavior for the image. | ||
| * - 'lazy': Defers loading until near the viewport | ||
| * - 'eager': Loads immediately | ||
| */ | ||
| @property({ type: String }) | ||
| public loading?: LoadingType; | ||
|
|
||
| /** | ||
| * How the image fits within the container. | ||
| * - 'contain': Scale to fit, maintain aspect ratio | ||
| * - 'cover': Scale to fill, maintain aspect ratio | ||
| * - 'fill': Stretch to fill | ||
| * - 'none': Do not resize | ||
| * - 'scale-down': Smaller of none or contain | ||
| */ | ||
| @property({ type: String, attribute: 'object-fit' }) | ||
| public objectFit?: ObjectFit; | ||
|
|
||
| /** | ||
| * Position of the image when using object-fit. | ||
| * Accepts CSS object-position values (e.g. 'center', 'top left'). | ||
| */ | ||
| @property({ type: String, attribute: 'object-position' }) | ||
| public objectPosition?: string; | ||
|
|
||
| /** | ||
| * Responsive image sources (e.g. "image-320w.jpg 320w, image-480w.jpg 480w"). | ||
| */ | ||
| @property({ type: String }) | ||
| public srcset?: string; | ||
|
|
||
| /** | ||
| * Sizes for responsive images (e.g. "(max-width: 600px) 480px, 800px"). | ||
| */ | ||
| @property({ type: String }) | ||
| public sizes?: string; | ||
|
|
||
| /** | ||
| * Image decoding hint: 'sync' | 'async' | 'auto'. | ||
| */ | ||
| @property({ type: String }) | ||
| public decoding?: DecodingType; | ||
|
|
||
| /** | ||
| * CORS setting for cross-origin images. | ||
| */ | ||
| @property({ type: String }) | ||
| public crossorigin?: CrossOrigin; | ||
|
|
||
| /** | ||
| * Referrer policy for image requests. | ||
| */ | ||
| @property({ type: String }) | ||
| public referrerpolicy?: ReferrerPolicy; | ||
|
|
||
| /** | ||
| * Width in pixels. Used for layout and aspect ratio. | ||
| */ | ||
| @property({ type: Number }) | ||
| public width?: number; | ||
|
|
||
| /** | ||
| * Height in pixels. Used for layout and aspect ratio. | ||
| */ | ||
| @property({ type: Number }) | ||
| public height?: number; | ||
|
|
||
| /** | ||
| * When true, shows an error state (e.g. failed to load). | ||
| */ | ||
| @property({ type: Boolean, reflect: true }) | ||
| public error = false; |
There was a problem hiding this comment.
swc-image has its own ImageBase with the exact same properties. This is a massive responsibility bloat on AssetBase. An swc-asset is conceptually a wrapperfor media. If you are making it also directly render elements means it's trying to be both a container and an image component simultaneously.
| * When true, shows an error state (e.g. failed to load). | ||
| */ | ||
| @property({ type: Boolean, reflect: true }) | ||
| public error = false; |
There was a problem hiding this comment.
How are you planning to create an automatic error detection when an image URL fails? This seems like a dead property. If consumer adds <swc-image error> nothing will happen. Kindly implement error state rendering in Image.ts.
| }) => { | ||
| const asset = await gotoStory( | ||
| page, | ||
| 'components-asset--media-wrapper', |
There was a problem hiding this comment.
Check this. This might fail.
| <img | ||
| part="image" | ||
| class="spectrum-Image-image" | ||
| src=${this.src} | ||
| alt=${this.alt || ''} | ||
| loading=${ifDefined(this.loading)} | ||
| decoding=${ifDefined(this.decoding)} | ||
| srcset=${ifDefined(this.srcset)} | ||
| sizes=${ifDefined(this.sizes)} | ||
| crossorigin=${ifDefined(this.crossorigin)} | ||
| referrerpolicy=${ifDefined(this.referrerpolicy)} | ||
| width=${ifDefined(this.width)} | ||
| height=${ifDefined(this.height)} | ||
| style=${styleMap(imageStyles)} | ||
| /> |
There was a problem hiding this comment.
There's no way for consumers to know when the image finishes loading. At minimum you can add a error state here like @error=${() => this.error = true} and dispatch a custom event like swc-image-error`.
| * Useful for grids and cards where thumbnails should share the same ratio. | ||
| */ | ||
| export const AspectRatio: Story = { | ||
| render: () => html` |
There was a problem hiding this comment.
Always spread ...args into template calls. This and Rounder stories are using render: () => (no args) and you are hardcoding everything which means Storybook controls are completely non-functional for these stories.
| export const Error: Story = { | ||
| args: { | ||
| error: true, | ||
| label: 'Failed to load', | ||
| }, | ||
| tags: ['states'], | ||
| }; |
There was a problem hiding this comment.
This is a very standalone error demonstration. can you be a more robust on this story and show
- Error with a variant set
- Error without a label
- Error with rounded corners
|
|
||
| protected override render(): TemplateResult { | ||
| if (!this.src) { | ||
| return html`<div class="spectrum-Image"></div>`; |
There was a problem hiding this comment.
An empty div takes up space in layout (the CSS gives it inline-size: 100% and block-size: 100%). Please consider rendering nothing so the empty state is intentional.
| .image-rounded::part(image) { | ||
| border-radius: 8px; | ||
| } | ||
| .image-circle::part(image) { | ||
| border-radius: 50%; | ||
| } |
There was a problem hiding this comment.
Don't use global style tags in storybook. It will leak unto other stries when SB reuses the DOM canvas. You can use a scoped decorator.
Rajdeepc
added
blocked
2 participants
Description
Asset (2nd gen)
New
swc-imagecomponent/tool (2nd-gen)image-only component: a container that renders an
<img>from src and exposes the inner image for styling (e.g. width, height, border-radius) via the image part and.spectrum-Image-imageclass.src(no slot).Motivation and context
This PR leads the discussion on what to do with the asset component now that the design dictionary in Spectrum 2 has diverged from its usage while the component is still being used in applications such as Photoshop Web.
Related issue(s)
Screenshots (if appropriate)
Author's checklist
Reviewer's checklist
patch,minor, ormajorfeaturesManual review test cases
Asset error state
swc-image
::part(image)styles apply (rounded corners and circle).Device review