feat: migrate color-loupe to gen2#6184
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 |
* feat(color-loupe): implement Color Loupe component with base functionality and styles * refactor(color-loupe): added migration planning and docs * fix(color-loupe): fixed styling * chore(color-loupe): add TODO for opacity-checkerboard migration * chore(color-loupe): reverting some changes * feat(color-loupe): enhance color loupe functionality and documentation * refactor(color-loupe): rename States story to OpenAndClosedStates and update documentation * refactor(color-loupe): update clip-path and SVG path for improved shape definition * fix(color-loupe): correct story identifiers in accessibility tests * chore(color-loupe): address PR review feedback - 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 * test(color-loupe): improve opacity assertions in OpenAttributeTest * refactor(color-loupe): refactored css according to styling guides * refactor(color-loupe): added default color to base * docs(color-loupe): update documentation for color loupe anatomy and inline CSS usage * chore: minor fix * refactor(color-loupe): improved stories * docs(color-loupe): reference existing validateColorString in @todo 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 * chore: minor fix Co-authored-by: Stephanie Eckles <seckles@adobe.com> * chore: added a line to remove linting error --------- Co-authored-by: Stephanie Eckles <seckles@adobe.com>
|
Adding the link for the PR to get context for changes |
| class=${classMap({ | ||
| ['swc-ColorLoupe']: true, | ||
| })} |
There was a problem hiding this comment.
| class=${classMap({ | |
| ['swc-ColorLoupe']: true, | |
| })} | |
| class="swc-ColorLoupe" | |
| })} |
There was a problem hiding this comment.
Thanks @rubencarvalho — you’re right that a plain class="swc-ColorLoupe" would be enough for a single, unconditional class.
I’m going to keep classMap on the root wrapper anyway so this line matches the same pattern we use in other 2nd-gen components (static single-class classMap on the top-level block) and stays consistent if we add conditional classes later (e.g. state/variant) without reworking the template. If we decide as a team to standardize on plain class for the non-conditional case, I’m happy to follow this up in a small cleanup PR.
There was a problem hiding this comment.
I disagree here. This should only be flattened to a class as @rubencarvalho suggested. Though this is not a merge blocker for now.
Yes let's discuss and if this becomes internal, the consumer guide will go away and @status and the stories file will change accordingly. The decision must land in this PR. The analogy I'd draw is color-handle — it's also a sub-component of the color picker family, but it's shipped as a standalone package precisely because consumers building custom color UIs (like Photoshop) need it independently. color-loupe sits in the same position: while we don't currently ship gradient picker components, a loupe is a natural fit for any gradient editor — whenever a user drags a color stop, you'd want a loupe to show the picked color without the finger/cursor obscuring it. Consumers building those experiences today would need to reach for the loupe on its own. Making it internal now would mean a breaking change later if we (or a downstream consumer) ever needs it outside the color picker context. Keeping it public is the safer default — it's a small, focused, self-contained component with a clear visual contract. |
miwha-adobe
left a comment
There was a problem hiding this comment.
Nice work! One other thing, can we add a changeset?
| * 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 |
There was a problem hiding this comment.
Can we include a ticket number here. What ticket is responsible for making sure this todo is completed?
There was a problem hiding this comment.
Good call — I've removed the @todo. Since swc-color-loupe always receives its color value from a parent color component (e.g. swc-color-area, swc-color-slider) that already validates the color string upstream via ColorController, there's no need to add a second validation layer here or track it as future work.
| opacity: 0; | ||
| filter: drop-shadow(token("drop-shadow-elevated-x") token("drop-shadow-elevated-y") token("drop-shadow-elevated-blur") token("drop-shadow-elevated-color")); | ||
|
|
||
| /* TODO: replace 8px with a forthcoming animation-distance token (matches Spectrum CSS S2). */ |
There was a problem hiding this comment.
Same with this todo. Do we have a ticket #, or is it a necessary todo? When will this animation token be available?
There was a problem hiding this comment.
The 8px value here corresponds to --mod-colorloupe-animation-distance in Spectrum CSS S2 (colorloupe/index.css), which controls how far the loupe translates down when it transitions to the closed state. That mod-property was intentionally not carried over as a consumer-facing hook (see the CSS custom properties section), but the underlying value also doesn't map to any token in the 2nd-gen token catalog yet.
I've left the TODO so we can swap it to a token("color-loupe-animation-distance") call once it lands in spectrum tokens, rather than silently burying a magic number.
There was a problem hiding this comment.
@5t3ph do you happen to have insight into when the s2 token will be carried over?
There was a problem hiding this comment.
You can remove the TODO, looking at the Spectrum CSS source, it seems this is in fact a magic number, and there's not a token planned. Looking at Color handle, the default gap value is 12px if we go by Figma: but Figma doesn't show a mapped token, just a static value.
If we find we need to modify it from the consuming components, we could expose a custom property to override the value.
There was a problem hiding this comment.
What would you recommend here? Should I remove the TODO and keep the hardcoded 8px value, or expose it as a custom property instead?
There was a problem hiding this comment.
Let's remove the TODO and keep the hardcoded value for now
|
|
||
| `<swc-color-loupe>` exposes no public CSS custom properties for consumer overrides. The component's visual appearance is fully governed by Spectrum 2 design tokens. | ||
|
|
||
| {/* @todo Replace the Description column with the `@cssproperty` JSDoc descriptions from `<swc-color-loupe>`'s CEM entry once they are added in a follow-up PR. */} |
There was a problem hiding this comment.
This todo seems like it can be removed. There are no css custom properties on this component as indicated in the Description section. Unless we anticipate this component being updated with them in which case we should have a ticket
| * color selection controls such as `<swc-color-field>`. | ||
| * | ||
| * @element swc-color-loupe | ||
| * @status preview |
There was a problem hiding this comment.
I believe this should be changed from "preview" to "unsupported"
…-components into color-loupe-migration
…-components into color-loupe-migration
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