feat(color-loupe): migrate sp-color-loupe component to 2nd-gen#6147
feat(color-loupe): migrate sp-color-loupe component to 2nd-gen#6147blunteshwar merged 21 commits intocolor-loupe-migrationfrom
Conversation
|
|
@blunteshwar Anything blocking this migration? |
📚 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 |
blunteshwar
changed the title
blunteshwar
added
Status:Ready for review
Rajdeepc
changed the title
5t3ph
left a comment
5t3ph
left a comment
There was a problem hiding this comment.
Requesting changes because it looks like the implementation could be upgraded to match S2 Spectrum CSS as noted.
| <div class="swc-ColorLoupe-checkerboard swc-ColorLoupe--clipped"></div> | ||
| <div | ||
| class="swc-ColorLoupe-colorFill swc-ColorLoupe--clipped" | ||
| style="background: ${this.color}" |
There was a problem hiding this comment.
I think we need to add a comment in the contributor docs stating to pass only CSS only strings from trusted sources.
There was a problem hiding this comment.
Good call. Added a new section "Inline CSS strings from component properties" to CONTRIBUTOR-DOCS/02_style-guide/02_typescript/09_rendering-patterns.md in commit fc8dbcf5bd.
Using this style="background: ${this.color}" line as the worked example, the new section codifies four rules:
- Only accept CSS strings from trusted sources
- Prefer structured inputs (
classMap+ design tokens) over free-form CSS - Scope values through a CSS custom property (
styleMap({'--swc-...': value})) rather than interpolating a full declaration - Document the trust contract on the property's JSDoc
It also includes ✅ preferred / ✅ acceptable /
There was a problem hiding this comment.
The second example it provides of passing this into the value of a custom property vs straight to the background would be preferred, and is the pattern also exemplified in the Spectrum CSS solution.
There was a problem hiding this comment.
Implemented. The template now routes this.color through a CSS custom property via styleMap, and the stylesheet consumes that property:
// ColorLoupe.ts
style=${styleMap({
'--swc-color-loupe-picked-color': this.color,
})}/* color-loupe.css */
.swc-ColorLoupe-colorFill {
background: var(--swc-color-loupe-picked-color);
}On HSV point — both background: ${this.color} and background: var(--swc-color-loupe-picked-color) hand the same string to the same browser CSS parser, so the set of accepted color formats is identical across the two patterns. The loupe always receives a CSS-valid string from its parent color picker, which is where non-CSS color-space conversion (including HSV → HSL/RGB) happens.
- ColorLoupe.ts: link TODO to SWC-2029 for opacity-checkerboard migration - color-loupe.test.ts: add CSS regression steps asserting inner-loupe opacity flips with [open] - package.json: revert unrelated customElements path change - vitest.config.js: revert unrelated coverage threshold bumps - CONTRIBUTOR-DOCS: document guidance for inline CSS strings from component properties Made-with: Cursor
5t3ph
left a comment
5t3ph
left a comment
There was a problem hiding this comment.
Thanks for addressing the render bug. A few remaining updates.
| block-size: token("color-loupe-height"); | ||
| pointer-events: none; | ||
| opacity: var(--_swc-color-loupe-opacity); | ||
| filter: drop-shadow(var(--swc-color-loupe-drop-shadow-x, token("drop-shadow-elevated-x")) var(--swc-color-loupe-drop-shadow-y, token("drop-shadow-elevated-y")) var(--swc-color-loupe-drop-shadow-blur, token("drop-shadow-elevated-blur")) var(--swc-color-loupe-drop-shadow-color, token("drop-shadow-elevated-color"))); |
There was a problem hiding this comment.
Per our guidelines, we are not exposing every token, only those that are actually modified by the component. This definition can be simplified to remove all of the exposed properties. Same for several other instances in this stylesheet.
I assume AI just converted the mod properties, but again, that is an anti-pattern.
There was a problem hiding this comment.
Done. Exposed --swc-color-loupe-* surface went from 10 properties down to 1 (only --swc-color-loupe-picked-color, which the component itself modifies on every render via the template).
Collapsed to direct token() calls (none modified by the component):
--swc-color-loupe-offset--swc-color-loupe-animation-distance--swc-color-loupe-drop-shadow-{x,y,blur,color}(×4)--swc-color-loupe-inner-border-{color,width}(×2)--swc-color-loupe-outer-border-width
The --swc-color-loupe-opacity / --swc-color-loupe-transform passthrough pair got replaced with literals on .swc-ColorLoupe, driven by :host([open]) .swc-ColorLoupe.
The outer-border-color is only modified in forced-colors mode, so per the exclusions guideline it's now an internal --_swc-color-loupe-outer-border-color shared by the normal rule and the forced-colors media query — same pattern status-light uses.
| display: block; | ||
| position: absolute; | ||
| inset-block-end: calc((token("color-handle-size") - token("color-handle-outer-border-width")) + var(--swc-color-loupe-offset, token("color-loupe-bottom-to-color-handle"))); | ||
| inset-inline-end: calc(50% - (token("color-loupe-width") / 2)); |
There was a problem hiding this comment.
Since color-loupe-width is referenced multiple times, you can hold this value in a private property.
There was a problem hiding this comment.
Done. Defined once as a private property on :host and referenced from all three sites:
:host {
--_swc-color-loupe-width: token("color-loupe-width");
...
inline-size: var(--_swc-color-loupe-width);
inset-inline-end: calc(50% - (var(--_swc-color-loupe-width) / 2));
}
:host(:dir(rtl)) {
inset-inline-end: calc(50% - (var(--_swc-color-loupe-width) / 2) - 1px);
}| <div class="swc-ColorLoupe-checkerboard swc-ColorLoupe--clipped"></div> | ||
| <div | ||
| class="swc-ColorLoupe-colorFill swc-ColorLoupe--clipped" | ||
| style="background: ${this.color}" |
There was a problem hiding this comment.
The second example it provides of passing this into the value of a custom property vs straight to the background would be preferred, and is the pattern also exemplified in the Spectrum CSS solution.
| * ### Technical structure | ||
| * | ||
| * The component has no slots. All rendering is internal. | ||
| * | ||
| * #### Properties | ||
| * | ||
| * - **open**: Controls visibility with animated CSS transitions on `opacity` and `transform` | ||
| * - **color**: CSS color string displayed inside the loupe; supports any valid format | ||
| * including named colors, hex, `rgba()`, and `hsl()` |
There was a problem hiding this comment.
This summary is against the established docs patterns, especially as the following stories are intended to describe and demonstrate the properties (and slots, if they were applicable).
The other issue is that having the "Properties" sub-heading breaks the table of contents link to the Properties table at the end.
I would advise removing this as essentially duplicate/unneeded information.
There was a problem hiding this comment.
agree, you can remove technical structure and properties sections. can you reference how the other components have documented their anatomy? this seems to be very different including that visual structure section
There was a problem hiding this comment.
Addressed across the anatomy rewrite and a broader stories consistency pass.
Anatomy JSDoc rewritten to match the established pattern used by asset, avatar, badge, divider, icon, progress-circle, and status-light — a short intro sentence plus a numbered list of visible parts:
/**
* A color loupe consists of:
*
* 1. **Floating loupe element** - A teardrop-shaped container positioned above the interaction point, with an inner and outer border
* 2. **Color preview** - Displays the currently picked color over an opacity checkerboard so transparency is visible
*/The #### Properties sub-heading that was colliding with the auto-generated Properties table's TOC anchor is gone, and so are the ### Visual structure / ### Technical structure sections.
Broader stories consistency pass brought the rest of the file in line with the reference components:
- Added a
HELPERSsection (mirrorsbadge.stories.ts) with a smalllabeledLoupehelper and aCOLOR_FORMATSlabel/color mapping - Replaced the inline wrapper-div boilerplate across
Colors/OpenAndClosedStates/ParentDrivenVisibilitywith.map()iteration over typed data (same shape as badge'sSizesstory) - Trimmed the meta JSDoc and dropped broken cross-component links to not-yet-migrated color components
- Added
.storyNameoverrides for sentence-case display in the sidebar - Fixed the
flexLayoutparameter value (true→'row-wrap'; the boolean form wasn't a valid case in theflex-layoutdecorator and was silently falling through todefault, which is why multi-loupe stories were rendering stacked vertically)
| // TODO: Migrate opacity-checkerboard to 2nd gen and consume it here; checkerboard styling is currently hardcoded in color-loupe.css. | ||
| // Tracked in SWC-2029. |
There was a problem hiding this comment.
nit:
| // TODO: Migrate opacity-checkerboard to 2nd gen and consume it here; checkerboard styling is currently hardcoded in color-loupe.css. | |
| // Tracked in SWC-2029. | |
| /** | |
| * @todo SWC-2029 - Migrate opacity-checkerboard to 2nd gen and consume it here; | |
| * checkerboard styling is currently hardcoded in color-loupe.css. | |
| */ |
There was a problem hiding this comment.
Applied verbatim:
/**
* @todo SWC-2029 - Migrate opacity-checkerboard to 2nd gen and consume it
* here; checkerboard styling is currently hardcoded in color-loupe.css.
*/
| * Default color value for a newly created color loupe. | ||
| * Semi-transparent red allows the opacity checkerboard to show through. | ||
| */ | ||
| export const COLOR_LOUPE_DEFAULT_COLOR = 'rgba(255, 0, 0, 0.5)'; |
There was a problem hiding this comment.
this isnt a type and can we defined in the base since its just a default value
There was a problem hiding this comment.
Good catch — you're right, it wasn't a type. Since COLOR_LOUPE_DEFAULT_COLOR was the only thing in ColorLoupe.types.ts and no associated enum/union justified its existence, I deleted the file entirely. The default now lives inline at the property declaration in the base class:
@property({ type: String })
public color = 'rgba(255, 0, 0, 0.5)';A short line in the property JSDoc explains the choice (semi-transparent red reveals the opacity checkerboard on initial render).
Also updated the index.ts barrel to drop the types re-export, and the migration checklist to note the file is intentionally absent with a link back to this thread for future reference.
| * transparency (which reveals the checkerboard behind). | ||
| */ | ||
| @property({ type: String }) | ||
| public color = COLOR_LOUPE_DEFAULT_COLOR; |
There was a problem hiding this comment.
We should be validating color is a valid CSS string and warn/error if its invalid. I could see a method similar to ifDefined that we can use across other components expecting CSS strings. isValidColoror isCSSColor something along those lines. could be established here or just go straight to core tools as a utility method.
There was a problem hiding this comment.
We might want to leave that to implementation to use their own validation to avoid any perf load and also they might accept a format that we don't include. Possibly a TODO to investigate later?
There was a problem hiding this comment.
when i say validating i mean string validation that it matches a color value structure. like does it match an rgba(0, 0, 0, 0) as a string for an example.
There was a problem hiding this comment.
We already have a method public validateColorString in ColorController.ts in reactive controllers. Since color-loupe always receives color from parent-component hence it does not use validateColorString method.
There was a problem hiding this comment.
ColorController.validateColorStringis the right hook if this ever needs standalone validation. Updated the @todo to reference the existing utility rather than proposing a new one, and made the reasoning explicit (the loupe delegates to its parent, which validates upstream):
/**
* The CSS color value to display inside the loupe.
* Supports any valid CSS color string, including those with alpha
* transparency (which reveals the checkerboard behind).
*
* Default is semi-transparent red so the opacity checkerboard is visible
* when the component is rendered without a `color` attribute.
*
* @todo Runtime validation is intentionally not performed here. The loupe
* always receives its color from a parent color-picker component, which
* validates upstream via `validateColorString` on `ColorController`. If
* the loupe ever needs standalone validation (e.g. consumed outside a
* parent color picker), reuse `ColorController.validateColorString`
* rather than adding a separate utility.
*/
| ## Inline CSS strings from component properties | ||
|
|
||
| Some components accept a property whose value is a CSS string — for example, `<swc-color-loupe>` exposes a `color` property that accepts any valid CSS color: | ||
|
|
||
| ```ts | ||
| // ColorLoupe.ts | ||
| <div | ||
| class="swc-ColorLoupe-colorFill swc-ColorLoupe--clipped" | ||
| style="background: ${this.color}" | ||
| ></div> | ||
| ``` | ||
|
|
||
| When interpolating a property directly into a `style` attribute, the value is forwarded verbatim to the browser's CSS parser. That means: | ||
|
|
||
| - **Only accept CSS strings from trusted sources.** Component properties that participate in an inline `style` (or a template string, or a `styleMap` entry) must be treated as trusted input. Do not expose such properties to arbitrary user-generated content without validation at the call site. | ||
| - **Prefer structured inputs wherever possible.** If a property has a small set of known-good values (variant, size, semantic color), drive styling through `classMap` and design tokens instead of a free-form CSS string. | ||
| - **Use a CSS custom property for the value, not a full declaration.** Set `--swc-<component>-<role>: ${value}` via `styleMap` rather than interpolating an entire declaration. This scopes what the property can affect to what the component's CSS explicitly consumes. | ||
| - **Document the contract on the property.** The property's JSDoc must state that the value is passed to the CSS parser as-is and that callers are responsible for ensuring it is a valid, trusted CSS value. | ||
|
|
||
| ```ts | ||
| // ✅ Preferred — structured input via classMap | ||
| class=${classMap({ | ||
| [`swc-Badge--${this.variant}`]: this.variant != null, | ||
| })} | ||
|
|
||
| // ✅ Acceptable — typed CSS property, scoped via a custom property | ||
| style=${styleMap({ | ||
| '--swc-color-loupe-picked-color': this.color, | ||
| })} | ||
|
|
||
| // ⚠️ Use with care — full inline declaration interpolated from a property. | ||
| // Only do this when the component's API contract explicitly declares | ||
| // the property as a trusted CSS string and the JSDoc documents that. | ||
| style="background: ${this.color}" | ||
| ``` |
There was a problem hiding this comment.
this section is confusing and seems a little contradictory. the example at the top should reflect the "acceptable" pattern. the class map shouldnt be included in this section since thats a class and not an inline style. the last one should actually be an example of a bad example and not to do.
There was a problem hiding this comment.
Reworked. Each of the three points addressed:
-
Top example now reflects the acceptable pattern — the intro code block leads with the
styleMap-plus-custom-property approach, followed by a CSS block showing the rule that consumes the property:style=${styleMap({ '--swc-color-loupe-picked-color': this.color, })}
.swc-ColorLoupe-colorFill { background: var(--swc-color-loupe-picked-color); }
-
ClassMap example removed — that's a class concern, not an inline-style concern, so both the code example and the prose bullet referencing
classMapare gone. The "prefer structured inputs" advice already lives in the dedicated "classMap patterns" section directly above this one. -
Last example is now ❌ Bad, not
⚠️ Use with care — plus a comment spelling out the actual cost: the full declaration is re-parsed on every render, and a malformed value can corrupt adjacent styles.
Section now has just two code examples: ✅ Good (styleMap + custom property) and ❌ Bad (interpolating into a full declaration).
Per review, validation delegation is already handled: the parent color-picker validates upstream via ColorController.validateColorString before passing to the loupe. Update the @todo to reference the existing utility rather than proposing a new one. Made-with: Cursor
5t3ph
left a comment
5t3ph
left a comment
There was a problem hiding this comment.
Noticed one last issue, but the other changes are great improvements!
Co-authored-by: Stephanie Eckles <seckles@adobe.com>
4 participants
Description
Migrates the
color-loupecomponent from 1st-gen (sp-color-loupe) to 2nd-gen (swc-color-loupe) following the washing machine workflow. All 7 migration steps are complete and the status table has been updated.What changed
Core package (
2nd-gen/packages/core/components/color-loupe/)ColorLoupe.types.ts— minimal types file with the default color constantColorLoupe.base.ts— abstract base class extendingSpectrumElement; holds the two public properties (open,color)index.ts— re-exportsSWC package (
2nd-gen/packages/swc/components/color-loupe/)ColorLoupe.ts— concrete class withrender()andstylescolor-loupe.css— token-based CSS following the S2 migration guideindex.ts—defineElement('swc-color-loupe', ColorLoupe)registrationstories/color-loupe.stories.ts— Playground, Overview, Anatomy, States, ColorDisplay (behaviors), and Accessibility storiestest/color-loupe.test.ts— Storybook play-function tests (defaults,openreflection,colorproperty)test/color-loupe.a11y.spec.ts— Playwright attribute test assertingaria-hidden="true"on the SVG (replaced the unusable emptytoMatchAriaSnapshotwhich Playwright rejects even for legitimately empty trees)Migration docs (
CONTRIBUTOR-DOCS/03_project-planning/03_components/color-loupe/)rendering-and-styling-migration-analysis.md— full API surface, DOM structure, factor assessment, and gen-2 delta notes (moved from1st-gen/packages/color-loupe/MIGRATION_ANALYSIS.md)accessibility-migration-analysis.md— updatedmigration-checklist.md— all 7 steps marked completeStatus table
CONTRIBUTOR-DOCS/.../01_status.md— all 7 columns marked for Color LoupeNotable changes from 1st-gen
61.575ZZ→61.575Z)xlink:href="#path"→href="#loupe-path")xlink:hrefxlink:hrefto modernhrefthroughout the SVG@spectrum-web-components/opacity-checkerboardimport with an inlinerepeating-conic-gradientusing--swc-opacity-checkerboard-*tokens andlight-dark()for theme support--spectrum-drop-shadow-xto S2token("drop-shadow-elevated-*")--spectrum-color-loupe-*/--mod-colorloupe-*chains replaced withtoken()and--swc-color-loupe-*exposed overridesdiv.swc-ColorLoupe-colorFillwithbackground: ${this.color}instead of setting--spectrum-picked-coloron the SVG inline styleMotivation and context
sp-color-loupeis a dependency of the color picker family. Migrating it to 2nd-gen unblocks downstream component migrations (swc-color-field,swc-color-slider,swc-color-area) and ensures it consumes Spectrum 2 design tokens.Related issue(s)
Screenshots (if appropriate)
Visual verification via the
color-loupe--overviewStorybook story. No screenshot attached; teardrop shape, checkerboard, and color fill are visually unchanged from 1st-gen.Author's checklist
Reviewer's checklist
patch,minor, ormajorfeaturesManual review test cases
Loupe renders correctly in Storybook
sp-color-loupeOpen/close animation works
opencontrol in the Storybook controls panelColor fill updates correctly
colorcontrol to a color with alpha (e.g.,rgba(255, 0, 0, 0.5))Lint and unit tests pass
yarn lint:2nd-genyarn test:unit:2ndPlaywright a11y tests pass
yarn test:a11y:2ndcolor-loupe.a11y.spec.tstests to pass — they assertaria-hidden="true"on the SVG elementDevice review
Accessibility testing checklist
Required: Complete each applicable item and document your testing steps.
Keyboard (non-interactive component — no focusable parts)
tabindexScreen reader (visual-only component — SVG is
aria-hidden)swc-color-loupecustom element itself is not announced as an interactive control