feat(core): migrate ColorController to 2nd-gen and optimize with colorjs.io#6450
feat(core): migrate ColorController to 2nd-gen and optimize with colorjs.io#6450blunteshwar wants to merge 6 commits into
Conversation
…rjs.io Relocate ColorController from 1st-gen reactive-controllers into @spectrum-web-components/core controllers, and optimize the implementation using colorjs.io built-ins without changing any output color formats. Changes: - Add controllers/color-controller (src + index) and export from controllers/index.ts; add colorjs.io@0.5.2 dependency, package exports, and typesVersions; externalize colorjs.io in the build. - Optimizations (no output-format change): hoist regex arrays to module constants, remove dead getColor formats (hsva/hsla are not real colorjs spaces and throw), dedupe space-id detection into a shared helper, centralize Dev Mode warnings behind a typeof-window guard for SSR/Node safety, and tighten internal types. - Port the controller test suite as a pure-logic Node unit test under test/__unit__, activating the dormant core-unit Vitest project. - Exclude test/__unit__ from the Storybook story indexer so unit tests are not indexed as CSF. 1st-gen ColorController and its consumers are untouched.
…r test Replace the Node-unit `test/__unit__/color-controller.test.ts` with a Storybook play test at `test/color-controller.test.ts`, matching the folder and file structure used by placement-controller. The assertions run as `play` functions in the storybook Vitest project against a minimal non-rendering host. Reverts the now-unneeded scaffolding: the dormant `core-unit` Vitest project in swc/vitest.config.js stays commented out, and the Storybook story indexer no longer needs a `__unit__` exclusion.
|
📚 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 |
Coverage Report for CI Build 28256271759Coverage remained the same at 96.246%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
Add a per-unit MDX docs page and Storybook stories for ColorController, mirroring the placement-controller structure: a `demo-color-playground` / `demo-color-formats` demo host, a stories file with `controller` meta tag and behavior stories, and an MDX page with What it does, Basic usage, Behaviors, Accessibility, a hand-authored API section, and an appendix.
pfulton
added
Status:Ready for review
nikkimk
left a comment
nikkimk
left a comment
There was a problem hiding this comment.
lgtm - the docs for this were helpful
| colorController.color = new Color('srgb', [0.5, 0.25, 0.75]); | ||
| colorController.colorOrigin = { r: '50%', g: '25%', b: '75%' }; | ||
| expect(colorController.colorValue).toEqual({ | ||
| r: '128%', | ||
| g: '64%', | ||
| b: '191%', | ||
| a: 1, | ||
| }); |
There was a problem hiding this comment.
Something is not adding up here how come these percentages are more than 100 lol... shouldn't this be
expect(colorController.colorValue).toEqual({
r: '50%',
g: '25%',
b: '75%',
a: 1,
});
There was a problem hiding this comment.
You are correct, In the color-controller while converting in % I was multiplying with 255 instead of 100.
Its fixed now
The srgb percentage-object branch of `colorValue` produced percentages above
100 (e.g. 0.5 -> "128%") because it multiplied the 0–1 channel by 255 (the
8-bit scale) and appended "%". It also used `String.search('%')` as a boolean,
which returns -1 (truthy) for non-percent strings, so the percent branch ran
for any string channel.
Multiply by 100 for percentages (0.5 -> "50%") and guard with `.includes('%')`.
This is a behavior fix to a long-standing Gen1 bug carried into the migration;
the test expectation is updated to the corrected `{ r: '50%', g: '25%', b: '75%' }`.
5 participants
Description
Relocates
ColorControllerfrom1st-genreactive-controllers into@spectrum-web-components/corecontrollers, and optimizes the implementation usingcolorjs.iobuilt-ins without changing any output color formats.This is a controller relocation plus code cleanup, not a component migration. The
1st-genColorControllerand its consumers (ColorSlider,ColorArea,ColorWheel,ColorField) are untouched.What changed
2nd-gen/packages/core/controllers/color-controller/(src/color-controller.ts+index.ts), exported fromcontrollers/index.ts.colorjs.io@0.5.2dependency (pinned to match 1st-gen), packageexports+typesVersionsentries, and externalizedcolorjs.ioin the build so it is not bundled into output.test/color-controller.test.ts, following the folder and file structure ofplacement-controller. Assertions run asplayfunctions against a minimal non-rendering host (6 stories cover the original 28 cases).Optimizations (no output-format change)
validateColorStringcall).getColorformats:hsva/hslaare not real colorjs.io spaces and throw on.to().colorsetter andcolorValuegetter.window.__swcwarning blocks into onedebugWarnhelper, guarded withtypeof windowso the controller is safe in SSR/Node environments.The format-sensitive logic (
colorValuestring building,_getHexStringforced-alpha) was intentionally kept manual: colorjs.io's owntoString()emits modern space-separated, significant-digit CSS and dropsalpha=1, which would break the pinned comma-separated /Math.round/ forced-alpha output.Motivation and context
The 2nd-gen
corepackage needsColorControlleravailable for the gen-2 color component migrations. Moving it now, with the long-standing implementation cleaned up, avoids carrying forward dead code and rebuilt-per-call regexes.Related issue(s)
Author's checklist
Reviewer's checklist
patch,minor, ormajorfeaturesManual review test cases
2nd-gen/packages/swc, runyarn vitest --run --project storybook color-controller.test.tsDevice review
Accessibility testing checklist
ColorControlleris a non-rendering reactive controller with no DOM, focusable parts, or screen-reader surface of its own.Keyboard (required — document steps below)
Screen reader (required — document steps below)