Charts: Support all CSS color formats in theme colors#46349
Charts: Support all CSS color formats in theme colors#46349adamwoodnz merged 13 commits intotrunkfrom
Conversation
|
Thank you for your PR! When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:
This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖 Follow this PR Review Process:
If you have questions about anything, reach out in #jetpack-developers for guidance! |
|
@copilot try to commit again. I have updated your firewall allowlist |
The commit has been successfully pushed! The branch is now up to date with all changes for CHARTS-138, including:
Commit: 7cd8804 |
|
@copilot the description needs to follow |
|
Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.
Interested in more tips and information?
|
Updated the PR description to follow the template format with proper sections including Fixes, Proposed changes, Testing instructions, and other required fields. |
Code Coverage SummaryCoverage changed in 2 files.
|
There was a problem hiding this comment.
Pull request overview
This PR adds support for HSL, RGB, and RGBA color formats in theme colors, where previously only hex colors were supported. This fix resolves an issue where CSS variables resolving to non-hex color formats were silently skipped during color cache building.
Key Changes:
- Extended
normalizeColorToHexto handlergba()colors in addition to existingrgb()andhsl()support - Refactored color cache building in
GlobalChartsProviderto usenormalizeColorToHexfor all color processing, ensuring consistent handling across all color formats - Added comprehensive test coverage for new RGBA format support and CSS variables resolving to RGB/HSL/RGBA values
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
projects/js-packages/charts/src/utils/color-utils.ts |
Added rgba() to the list of supported color formats in normalizeColorToHex and updated documentation in parseRgbString to clarify it only handles RGB (not RGBA) |
projects/js-packages/charts/src/utils/test/color-utils.test.ts |
Added comprehensive tests for RGBA color conversion including edge cases with alpha channel and fully transparent colors |
projects/js-packages/charts/src/providers/chart-context/global-charts-provider.tsx |
Refactored color cache building to use normalizeColorToHex for all color processing, replacing manual CSS variable resolution with centralized normalization |
projects/js-packages/charts/src/providers/chart-context/test/chart-context.test.tsx |
Added integration tests for CSS variables resolving to RGB, HSL, and RGBA formats, and updated existing test expectations to reflect 3-digit hex expansion behavior |
projects/js-packages/charts/changelog/charts-138-support-hsl-rgb-rgba-colors |
Added changelog entry documenting the new color format support |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
7cd8804 to
4a8c502
Compare
4a8c502 to
0360588
Compare
|
Claude finished @adamwoodnz's task in 48s —— View job Code Review
The two issues flagged in the previous review have both been addressed in subsequent commits. The PR is now in good shape. Previous issues — resolved
Dead code guard in Remaining minor noteThe OverallThe implementation is clean, the centralization in |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Comments suppressed due to low confidence (1)
projects/js-packages/charts/src/utils/color-utils.ts:200
- normalizeColorToHex() currently only calls d3-color for hsl()/hsla()/rgb()/rgba() strings. Named CSS colors (e.g. "steelblue") and other valid CSS color syntaxes will fall through to the "Unknown format" path and be returned as-is, which means GlobalChartsProvider will skip them when building the palette (it only keeps values starting with "#"). If the intent is to support named colors (as implied by the Storybook theme/PR description), add a fallback that attempts d3Color(trimmed) for non-hex, non-CSS-var inputs and returns formatHex() when parsing succeeds, plus a unit test for at least one named color.
// Handle HSL, HSLA, RGB, and RGBA strings using d3-color
if (
trimmed.startsWith( 'hsl(' ) ||
trimmed.startsWith( 'hsla(' ) ||
trimmed.startsWith( 'rgb(' ) ||
trimmed.startsWith( 'rgba(' )
) {
const parsed = d3Color( trimmed );
if ( parsed ) {
return parsed.formatHex();
}
return color;
}
// Unknown format, return as-is
return color;
};
projects/js-packages/charts/changelog/charts-138-support-hsl-rgb-rgba-colors
Outdated
Show resolved
Hide resolved
projects/js-packages/charts/src/providers/chart-context/global-charts-provider.tsx
Show resolved
Hide resolved
Alpha blending with a background color can't be done reliably since we don't know the actual rendered background. Keep the simpler alpha-stripping behavior from d3-color's formatHex(). Use a pre-blended RGB value for the leaderboard secondary color in the mixed-color-formats theme so it's visually distinct from the primary. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Named colors like "steelblue" were falling through to the "unknown format" path and returned as-is. Since the provider only keeps values starting with "#", these were silently dropped from the palette. Add a d3-color fallback for any unrecognized format before returning as-is. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
If resolveCss returns the original input unchanged (e.g. due to case mismatch between "VAR(--x)" and "var(--x)"), normalizeColorToHex would recurse infinitely. Skip recursion when resolved === color. Also update the docblock to reflect all supported formats. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
@claude final review please |
|
Claude finished @adamwoodnz's task in 54s —— View job Final Code Review
All issues from the two previous rounds of review have been addressed. The PR is ready to merge. Previous issues — all resolved
One minor noteThe changelog text ( OverallThe implementation is clean and correct. |
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Changelog updated in a66dd11 |
dognose24
left a comment
There was a problem hiding this comment.
Do we also need to add tests for the new logic resolved !== color?
it( 'returns original when CSS variable resolves to itself', () => {
const mockResolve = jest.fn().mockImplementation( v => v );
expect( normalizeColorToHex( '--loop', null, mockResolve ) ).toBe( '--loop' );
} );
| } ); | ||
| } ); | ||
|
|
||
| describe( 'Named CSS colors', () => { |
The existing `resolved !== color` check only prevented direct self-references (--a resolving to --a). Indirect cycles like --a → var(--b) → --a would still cause a stack overflow. Add a depth counter to cap recursion at 10 levels, which is more than sufficient for real-world CSS variable chains. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Covers the direct self-reference case where resolveCss returns the same value it was given, verifying the `resolved !== color` guard. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Cover whitespace trimming, case-insensitive color function parsing (HSL, RGB, RGBA, VAR), and multi-hop CSS variable resolution chains. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Thanks, covered these cases and a bunch of other edge cases with tests in 3 new commits. |
Fixes https://linear.app/a8c/issue/CHARTS-138/support-hsl-rgb-and-rgba-theme-colors
Proposed changes:
normalizeColorToHexto handlehsla(),rgba(), and named CSS colors (e.g.steelblue) via d3-color fallbackGlobalChartsProviderto usenormalizeColorToHexfor all color processing, removing an unreachable guardparseRgbString— redundant now thatnormalizeColorToHexcovers all formatsOther information:
Jetpack product discussion
https://linear.app/a8c/review/charts-add-global-theme-support-for-generating-colors-from-a-single-842566402164#comment-ce4ba572
Does this pull request change what data or activity we track or use?
No
Testing instructions:
Setup
cd projects/js-packages/chartspnpm installVerify the fix
pnpm testcolor-utils.test.tsand RGB/HSL/RGBA tests inchart-context.test.tsxpnpm run buildpnpm run typecheckVerify in Storybook
mixed-color-formatstheme from the theme selectorsteelblue)Technical details
Before this change: CSS variables resolving to
rgb(),rgba(),hsl(),hsla(), or named colors were silently skipped during color cache building because the code only checked for colors starting with#.After this change: All color formats are normalized using
normalizeColorToHex, which:resolveCssVariable#bad→#bbaadd)steelblue→#4682b4) via d3-color fallbackNote: Alpha channel information in RGBA/HSLA colors is stripped during hex conversion (d3-color
formatHex()behavior). Fully transparent colors (alpha=0) convert to#000000.