Conversation
Pre-compute hex colors synchronously in the useState initializer to prevent the color flicker that occurs when colorCache starts empty and gets populated after the first render.
|
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 🤖 🔴 Action required: We would recommend that you add a section to the PR description to specify whether this PR includes any changes to data or privacy, like so: Follow this PR Review Process:
If you have questions about anything, reach out in #jetpack-developers for guidance! |
There was a problem hiding this comment.
Pull request overview
This PR fixes a visual bug where chart colors briefly flicker from an incorrect red color (#813131) to the correct theme color (e.g., #98C8DF) during the initial render of the Conversion Funnel Chart and other charts.
Changes:
- Pre-compute hex colors synchronously in the
useStateinitializer to populate the color cache before the first render - Retain the
useLayoutEffectto handle CSS variable resolution after DOM is ready - Add changelog entry documenting the fix
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
projects/js-packages/charts/src/providers/chart-context/global-charts-provider.tsx |
Modified useState initializer to pre-compute hex colors synchronously, preventing color flicker on initial render |
projects/js-packages/charts/changelog/fix-charts-color-flicker |
Added changelog entry documenting the fix |
Verifies that hex colors are immediately available on the first render without showing a fallback color (fixing the flicker issue).
Instead of pre-computing hex colors in useState initializer (duplicating logic from useLayoutEffect), add a fallback path in resolveColor that checks if colorCache is empty and uses raw theme colors directly. This is cleaner as it keeps color processing logic in one place.
projects/js-packages/charts/src/providers/chart-context/global-charts-provider.tsx
Show resolved
Hide resolved
| // Fallback for first render: if colorCache is not yet populated by useLayoutEffect, | ||
| // use raw theme hex colors directly to prevent color flicker | ||
| const getRawThemeColor = ( colorIndex: number ): string | null => { | ||
| if ( colorCache.colors.length === 0 ) { | ||
| const themeColor = providerTheme.colors?.[ colorIndex ]; | ||
| if ( themeColor && typeof themeColor === 'string' && themeColor.startsWith( '#' ) ) { | ||
| return themeColor; | ||
| } | ||
| } | ||
| return null; | ||
| }; |
There was a problem hiding this comment.
The getRawThemeColor function is defined inside the useCallback for resolveColor, which means it gets recreated on every render. This is inefficient and goes against the purpose of using useCallback for performance optimization.
Consider moving this function outside of the useCallback, or better yet, implementing the solution described in the PR description: pre-computing hex colors directly in the useState initializer. This would eliminate the need for this runtime fallback function entirely and would be more performant.
There was a problem hiding this comment.
Copilot has a point about the function being recreated. However, getRawThemeColor is only created when resolveColor itself is recreated (when dependencies change), not on every render. The performance impact is minimal.
projects/js-packages/charts/src/providers/chart-context/global-charts-provider.tsx
Show resolved
Hide resolved
Code Coverage SummaryCoverage changed in 1 file.
|
projects/js-packages/charts/src/providers/chart-context/global-charts-provider.tsx
Outdated
Show resolved
Hide resolved
…-charts-provider.tsx Co-authored-by: Adam Wood <1017872+adamwoodnz@users.noreply.github.com>
Fixes https://linear.app/a8c/issue/CHARTS-168
Proposed changes:
Fix the color flicker issue in Conversion Funnel Chart (and other charts) where bars briefly show a red color (
#813131) before switching to the correct theme color (#98C8DF).Root Cause Analysis
When the
GlobalChartsProvidercomponent first renders, it needs a color palette to know what colors to use. The palette was being prepared in two steps:The problem is that React renders the component BEFORE
useLayoutEffectruns.So on the first render:
Is 0 < 0?(is index less than array length?)#813131(a reddish color)Then
useLayoutEffectruns, populates the real colors, triggers a re-render, and nowcolors[0]returns#98C8DF.The Fix
Add a fallback path in
resolveColorthat checks ifcolorCacheis empty and uses raw theme hex colors directly:Now on the first render, when
colorCacheis empty, we fall back to the raw theme colors directly. No flicker!Why useLayoutEffect Still Exists
Some colors might be CSS variables like
var(--my-color). These can't be resolved until the DOM exists. SouseLayoutEffecthandles those cases, but for normal hex colors, the fallback provides immediate access.Other information:
Testing instructions:
projects/js-packages/chartspnpm storybook(or from monorepo root:pnpm --filter @automattic/charts storybook)#813131(red) before settling on#98C8DF(blue)