-
Notifications
You must be signed in to change notification settings - Fork 834
Charts: generate extra colors from color palette #45276
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Charts: generate extra colors from color palette #45276
Conversation
Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.
Interested in more tips and information?
|
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! |
Code Coverage SummaryCoverage changed in 2 files.
1 file is newly checked for coverage.
|
datum={ sampleData[ 1 ].data[ sampleData[ 1 ].data.length - 10 ] } | ||
datum={ sampleData[ 1 ].data[ 1 ] } | ||
title="Another notable event" | ||
subtitle="This is another notable event" | ||
{ ...( annotationArgs?.[ 1 ] || {} ) } | ||
/> | ||
<LineChart.Annotation | ||
datum={ sampleData[ 2 ].data[ sampleData[ 2 ].data.length - 51 ] } | ||
datum={ sampleData[ 2 ].data[ 7 ] } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When the sample data was updated sometime recently these indexes broke. Fixing here as I'm touching the sample data again.
866c1b2
to
dd36607
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds automatic color generation for charts when the number of data series exceeds the available theme colors. Instead of cycling through the existing palette, the system now generates complementary colors using the golden ratio and perceptual distance algorithms to ensure good visual distinction and accessibility.
- Implements color generation algorithms using HSL color space with distance calculations
- Adds performance optimizations through color caching in the chart context
- Extends sample data and stories to demonstrate charts with 8+ series
Reviewed Changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 4 comments.
Show a summary per file
File | Description |
---|---|
projects/js-packages/charts/src/utils/color-utils.ts | Adds hexToHsl and getColorDistance functions for color conversion and perceptual distance calculation |
projects/js-packages/charts/src/utils/test/color-utils.test.ts | Comprehensive test suite for new color utility functions |
projects/js-packages/charts/src/providers/chart-context/private/get-chart-color.ts | Core color generation logic using golden ratio and distance algorithms |
projects/js-packages/charts/src/providers/chart-context/global-charts-provider.tsx | Integrates color caching and generation into the chart context |
projects/js-packages/charts/src/providers/chart-context/test/chart-context.test.tsx | Updates tests to verify color generation behavior instead of cycling |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
projects/js-packages/charts/src/providers/chart-context/private/get-chart-color.ts
Outdated
Show resolved
Hide resolved
projects/js-packages/charts/src/providers/chart-context/private/get-chart-color.ts
Outdated
Show resolved
Hide resolved
projects/js-packages/charts/src/providers/chart-context/private/get-chart-color.ts
Outdated
Show resolved
Hide resolved
for ( let attempt = 0; attempt < maxAttempts; attempt++ ) { | ||
let hue = ( ( index - colors.length + attempt * 0.1 ) * goldenRatio * 360 ) % 360; | ||
|
||
// If we have existing colors, constrain new colors to their hue range |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hue range for Jetpack is wide (lots of different colors), whereas hue range is small for Woo (all blue/purple/pink). Consider this when generating.
- Introduced constants for golden ratio, minimum color distance, and maximum generation attempts to improve readability and maintainability. - Added detailed comments to clarify the purpose of each constant and the overall color generation process.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me!
// Process all colors once and cache the results | ||
if ( Array.isArray( colors ) ) { | ||
for ( const color of colors ) { | ||
if ( color && typeof color === 'string' && color.startsWith( '#' ) ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens for named colors, like red
, dodgerblue
etc? if we don't not support them then we should probably be clear in the documentation.
@adamwoodnz just dumping some thoughts on next steps here - I don't think there would be a lot of use cases where we need more series than 10, so it's definitely not a priority right now. We could use combinations of color, glyph and pattern to multiply the choice of series / bar styles. For example, if we have 5 colors, 4 glyphs and 3 patterns, we'd have 5x4x3=60 combinations! It'll be impossible for consumers to exceed the limit 😄 |
Fixes CHARTS-107: Generate extra colors for charts from primary color
Proposed changes:
Screenshots
In both cases the first 5 colors are from
theme.colors
Other information:
Jetpack product discussion
Does this pull request change what data or activity we track or use?
Testing instructions: