From ec8b29b7721a89159e862054b3133b3a8e30402d Mon Sep 17 00:00:00 2001 From: Adam Wood <1017872+adamwoodnz@users.noreply.github.com> Date: Wed, 17 Sep 2025 15:06:54 +1200 Subject: [PATCH 01/23] Use a constant for chart types --- .../src/components/bar-chart/bar-chart.tsx | 3 ++- .../leaderboard-chart/leaderboard-chart.tsx | 3 ++- .../src/components/line-chart/line-chart.tsx | 3 ++- .../src/components/pie-chart/pie-chart.tsx | 3 ++- .../pie-semi-circle-chart.tsx | 3 ++- .../src/providers/chart-context/constants.ts | 10 ++++++++++ .../hooks/use-chart-registration.ts | 3 ++- .../src/providers/chart-context/index.ts | 3 ++- .../chart-context/test/chart-context.test.tsx | 20 +++++++++---------- .../src/providers/chart-context/types.ts | 4 +++- 10 files changed, 37 insertions(+), 18 deletions(-) create mode 100644 projects/js-packages/charts/src/providers/chart-context/constants.ts diff --git a/projects/js-packages/charts/src/components/bar-chart/bar-chart.tsx b/projects/js-packages/charts/src/components/bar-chart/bar-chart.tsx index f042875744e3a..c492073163875 100644 --- a/projects/js-packages/charts/src/components/bar-chart/bar-chart.tsx +++ b/projects/js-packages/charts/src/components/bar-chart/bar-chart.tsx @@ -16,6 +16,7 @@ import { useChartRegistration, useGlobalChartsContext, GlobalChartsContext, + ChartTypes, } from '../../providers'; import { attachSubComponents } from '../../utils'; import { Legend, useChartLegendItems } from '../legend'; @@ -277,7 +278,7 @@ const BarChartInternal: FC< BarChartProps > = ( { useChartRegistration( { chartId, legendItems, - chartType: 'bar', + chartType: ChartTypes.Bar, isDataValid, metadata: chartMetadata, } ); diff --git a/projects/js-packages/charts/src/components/leaderboard-chart/leaderboard-chart.tsx b/projects/js-packages/charts/src/components/leaderboard-chart/leaderboard-chart.tsx index 75da420cdd9b9..3c73ff86b6188 100644 --- a/projects/js-packages/charts/src/components/leaderboard-chart/leaderboard-chart.tsx +++ b/projects/js-packages/charts/src/components/leaderboard-chart/leaderboard-chart.tsx @@ -15,6 +15,7 @@ import { useChartRegistration, useGlobalChartsContext, useGlobalChartsTheme, + ChartTypes, } from '../../providers'; import { formatMetricValue, attachSubComponents } from '../../utils'; import { Legend } from '../legend'; @@ -194,7 +195,7 @@ const LeaderboardChartInternal: FC< LeaderboardChartProps > = ( { useChartRegistration( { chartId, legendItems, - chartType: 'leaderboard', + chartType: ChartTypes.Leaderboard, isDataValid, metadata: chartMetadata, } ); diff --git a/projects/js-packages/charts/src/components/line-chart/line-chart.tsx b/projects/js-packages/charts/src/components/line-chart/line-chart.tsx index 289c167cf8b8a..751cc8f924418 100644 --- a/projects/js-packages/charts/src/components/line-chart/line-chart.tsx +++ b/projects/js-packages/charts/src/components/line-chart/line-chart.tsx @@ -18,6 +18,7 @@ import { useChartRegistration, useGlobalChartsContext, useGlobalChartsTheme, + ChartTypes, } from '../../providers'; import { attachSubComponents, getSeriesLineStyles } from '../../utils'; import { Legend, useChartLegendItems } from '../legend'; @@ -353,7 +354,7 @@ const LineChartInternal = forwardRef< SingleChartRef, LineChartProps >( useChartRegistration( { chartId, legendItems, - chartType: 'line', + chartType: ChartTypes.Line, isDataValid, metadata: chartMetadata, } ); diff --git a/projects/js-packages/charts/src/components/pie-chart/pie-chart.tsx b/projects/js-packages/charts/src/components/pie-chart/pie-chart.tsx index 6b2d4badbf071..b2aafbc91a03d 100644 --- a/projects/js-packages/charts/src/components/pie-chart/pie-chart.tsx +++ b/projects/js-packages/charts/src/components/pie-chart/pie-chart.tsx @@ -10,6 +10,7 @@ import { useGlobalChartsContext, useGlobalChartsTheme, GlobalChartsContext, + ChartTypes, } from '../../providers'; import { attachSubComponents } from '../../utils'; import { getStringWidth } from '../../visx/text'; @@ -172,7 +173,7 @@ const PieChartInternal = ( { useChartRegistration( { chartId, legendItems, - chartType: 'pie', + chartType: ChartTypes.Pie, isDataValid: isValid, metadata: chartMetadata, } ); diff --git a/projects/js-packages/charts/src/components/pie-semi-circle-chart/pie-semi-circle-chart.tsx b/projects/js-packages/charts/src/components/pie-semi-circle-chart/pie-semi-circle-chart.tsx index cae6c9e88e4cb..31d67d514bbe1 100644 --- a/projects/js-packages/charts/src/components/pie-semi-circle-chart/pie-semi-circle-chart.tsx +++ b/projects/js-packages/charts/src/components/pie-semi-circle-chart/pie-semi-circle-chart.tsx @@ -12,6 +12,7 @@ import { useChartRegistration, useGlobalChartsContext, GlobalChartsContext, + ChartTypes, } from '../../providers'; import { attachSubComponents } from '../../utils'; import { Legend, useChartLegendItems } from '../legend'; @@ -204,7 +205,7 @@ const PieSemiCircleChartInternal: FC< PieSemiCircleChartProps > = ( { useChartRegistration( { chartId, legendItems, - chartType: 'pie-semi-circle', + chartType: ChartTypes.PieSemiCircle, isDataValid: isValid, metadata: chartMetadata, } ); diff --git a/projects/js-packages/charts/src/providers/chart-context/constants.ts b/projects/js-packages/charts/src/providers/chart-context/constants.ts new file mode 100644 index 0000000000000..03fce921f9ab4 --- /dev/null +++ b/projects/js-packages/charts/src/providers/chart-context/constants.ts @@ -0,0 +1,10 @@ +/** + * Constants for all supported chart types in the charts package + */ +export const ChartTypes = { + Bar: 'bar', + Leaderboard: 'leaderboard', + Line: 'line', + Pie: 'pie', + PieSemiCircle: 'pie-semi-circle', +} as const; diff --git a/projects/js-packages/charts/src/providers/chart-context/hooks/use-chart-registration.ts b/projects/js-packages/charts/src/providers/chart-context/hooks/use-chart-registration.ts index 52786a4fddd0d..9148e27326f4b 100644 --- a/projects/js-packages/charts/src/providers/chart-context/hooks/use-chart-registration.ts +++ b/projects/js-packages/charts/src/providers/chart-context/hooks/use-chart-registration.ts @@ -2,6 +2,7 @@ import { useEffect, useMemo } from 'react'; import { useDeepMemo } from '../../../hooks'; import { useGlobalChartsContext } from './use-global-charts-context'; import type { BaseLegendItem } from '../../../components/legend'; +import type { ChartType } from '../../../types'; export const useChartRegistration = ( { chartId, @@ -12,7 +13,7 @@ export const useChartRegistration = ( { }: { chartId: string; legendItems: BaseLegendItem[]; - chartType: string; + chartType: ChartType; isDataValid: boolean; metadata?: Record< string, unknown >; } ): void => { diff --git a/projects/js-packages/charts/src/providers/chart-context/index.ts b/projects/js-packages/charts/src/providers/chart-context/index.ts index bd89ef2f46737..55cf7b4c3dd9e 100644 --- a/projects/js-packages/charts/src/providers/chart-context/index.ts +++ b/projects/js-packages/charts/src/providers/chart-context/index.ts @@ -3,5 +3,6 @@ export { useGlobalChartsContext } from './hooks/use-global-charts-context'; export { useChartId } from './hooks/use-chart-id'; export { useChartRegistration } from './hooks/use-chart-registration'; export { useGlobalChartsTheme } from './hooks/use-global-charts-theme'; -export type { GlobalChartsContextValue, ChartRegistration } from './types'; +export { ChartTypes } from './constants'; +export type { GlobalChartsContextValue, ChartRegistration, ChartType } from './types'; export * from './themes'; diff --git a/projects/js-packages/charts/src/providers/chart-context/test/chart-context.test.tsx b/projects/js-packages/charts/src/providers/chart-context/test/chart-context.test.tsx index 6936aa2ca3e7a..4cd2b67bada6a 100644 --- a/projects/js-packages/charts/src/providers/chart-context/test/chart-context.test.tsx +++ b/projects/js-packages/charts/src/providers/chart-context/test/chart-context.test.tsx @@ -1,6 +1,6 @@ import { render } from '@testing-library/react'; import { useMemo } from 'react'; -import { GlobalChartsProvider } from '../global-charts-provider'; +import { GlobalChartsProvider, ChartTypes } from '../global-charts-provider'; import { useChartId } from '../hooks/use-chart-id'; import { useChartRegistration } from '../hooks/use-chart-registration'; import { useGlobalChartsContext } from '../hooks/use-global-charts-context'; @@ -107,7 +107,7 @@ describe( 'ChartContext', () => { useChartRegistration( { chartId, legendItems: mockLegendItems, - chartType: 'bar', + chartType: ChartTypes.Bar, isDataValid: true, metadata, } ); @@ -124,7 +124,7 @@ describe( 'ChartContext', () => { const chartData = contextValue.getChartData( 'test-chart' ); expect( chartData ).toEqual( { legendItems: mockLegendItems, - chartType: 'bar', + chartType: ChartTypes.Bar, metadata: { test: true }, } ); } ); @@ -140,13 +140,13 @@ describe( 'ChartContext', () => { useChartRegistration( { chartId: chartId1, legendItems: mockLegendItems, - chartType: 'bar', + chartType: ChartTypes.Bar, isDataValid: true, } ); useChartRegistration( { chartId: chartId2, legendItems: mockLegendItems, - chartType: 'line', + chartType: ChartTypes.Line, isDataValid: true, } ); @@ -160,8 +160,8 @@ describe( 'ChartContext', () => { ); expect( contextValue.charts.size ).toBe( 2 ); - expect( contextValue.getChartData( 'chart-1' )?.chartType ).toBe( 'bar' ); - expect( contextValue.getChartData( 'chart-2' )?.chartType ).toBe( 'line' ); + expect( contextValue.getChartData( 'chart-1' )?.chartType ).toBe( ChartTypes.Bar ); + expect( contextValue.getChartData( 'chart-2' )?.chartType ).toBe( ChartTypes.Line ); } ); it( 'returns undefined for non-existent charts', () => { @@ -192,14 +192,14 @@ describe( 'ChartContext', () => { useChartRegistration( { chartId, legendItems: mockLegendItems, - chartType: 'bar', + chartType: ChartTypes.Bar, isDataValid: true, } ); // Register second chart with same ID useChartRegistration( { chartId, legendItems: mockLegendItems, - chartType: 'line', + chartType: ChartTypes.Line, isDataValid: true, } ); @@ -213,7 +213,7 @@ describe( 'ChartContext', () => { ); expect( contextValue.charts.size ).toBe( 1 ); - expect( contextValue.getChartData( 'same-id' )?.chartType ).toBe( 'line' ); + expect( contextValue.getChartData( 'same-id' )?.chartType ).toBe( ChartTypes.Line ); } ); } ); diff --git a/projects/js-packages/charts/src/providers/chart-context/types.ts b/projects/js-packages/charts/src/providers/chart-context/types.ts index 574576c45b48d..3f9c4f16c2426 100644 --- a/projects/js-packages/charts/src/providers/chart-context/types.ts +++ b/projects/js-packages/charts/src/providers/chart-context/types.ts @@ -1,9 +1,11 @@ +import type { ChartTypes } from './constants'; import type { BaseLegendItem } from '../../components/legend'; import type { CompleteChartTheme } from '../../types'; +export type ChartType = keyof typeof ChartTypes; export interface ChartRegistration { legendItems: BaseLegendItem[]; - chartType: string; + chartType: ChartType; metadata?: Record< string, unknown >; } From cf321048bae47234b64709fcb1354846092e6aeb Mon Sep 17 00:00:00 2001 From: Adam Wood <1017872+adamwoodnz@users.noreply.github.com> Date: Wed, 17 Sep 2025 16:50:45 +1200 Subject: [PATCH 02/23] Add getElementStyles method with color and lineStyles properties --- .../src/components/bar-chart/bar-chart.tsx | 28 +++----- .../hooks/use-leaderboard-legend-items.ts | 8 +-- .../leaderboard-chart/leaderboard-chart.tsx | 6 +- .../components/legend/private/base-legend.tsx | 13 ++-- .../src/components/line-chart/line-chart.tsx | 30 ++++---- .../src/components/pie-chart/pie-chart.tsx | 8 ++- .../pie-semi-circle-chart.tsx | 8 +-- .../chart-context/global-charts-provider.tsx | 30 ++++++-- .../chart-context/test/chart-context.test.tsx | 68 +++++++++---------- .../src/providers/chart-context/types.ts | 28 ++++++-- 10 files changed, 128 insertions(+), 99 deletions(-) diff --git a/projects/js-packages/charts/src/components/bar-chart/bar-chart.tsx b/projects/js-packages/charts/src/components/bar-chart/bar-chart.tsx index c492073163875..f4144c72a3ad6 100644 --- a/projects/js-packages/charts/src/components/bar-chart/bar-chart.tsx +++ b/projects/js-packages/charts/src/components/bar-chart/bar-chart.tsx @@ -127,24 +127,15 @@ const BarChartInternal: FC< BarChartProps > = ( { totalPoints, } ); - const { resolveGroupColor } = useGlobalChartsContext(); - - const getColor = useCallback( - ( seriesData: SeriesData, index: number ) => - resolveGroupColor( { - group: seriesData.group, - index, - overrideColor: seriesData.options?.stroke, - } ), - [ resolveGroupColor ] - ); + const { getElementStyles } = useGlobalChartsContext(); const getBarBackground = useCallback( - ( index: number ) => () => - withPatterns + ( index: number ) => () => { + return withPatterns ? `url(#${ getPatternId( chartId, index ) })` - : getColor( dataSorted[ index ], index ), - [ withPatterns, getColor, dataSorted, chartId ] + : getElementStyles( { data: dataSorted[ index ], index, chartType: ChartTypes.Bar } ).color; + }, + [ withPatterns, getElementStyles, dataSorted, chartId ] ); const renderDefaultTooltip = useCallback( @@ -342,12 +333,15 @@ const BarChartInternal: FC< BarChartProps > = ( { <> { dataSorted.map( ( seriesData, index ) => - renderPattern( index, getColor( seriesData, index ) ) + renderPattern( index, getElementStyles( { data: seriesData, index } ).color ) ) } diff --git a/projects/js-packages/charts/src/components/leaderboard-chart/hooks/use-leaderboard-legend-items.ts b/projects/js-packages/charts/src/components/leaderboard-chart/hooks/use-leaderboard-legend-items.ts index 6f5a7fb74cae2..832aad283a94f 100644 --- a/projects/js-packages/charts/src/components/leaderboard-chart/hooks/use-leaderboard-legend-items.ts +++ b/projects/js-packages/charts/src/components/leaderboard-chart/hooks/use-leaderboard-legend-items.ts @@ -36,7 +36,7 @@ export function useLeaderboardLegendItems( { }; } ): BaseLegendItem[] { const { leaderboardChart: leaderboardChartSettings } = useGlobalChartsTheme(); - const { resolveGroupColor } = useGlobalChartsContext(); + const { getElementStyles } = useGlobalChartsContext(); return useMemo( () => { if ( ! data || data.length === 0 ) { @@ -46,7 +46,7 @@ export function useLeaderboardLegendItems( { const items: BaseLegendItem[] = []; // Add current period legend item - const resolvedPrimaryColor = resolveGroupColor( { + const { color: resolvedPrimaryColor } = getElementStyles( { index: 0, overrideColor: primaryColor || leaderboardChartSettings.primaryColor, } ); @@ -61,7 +61,7 @@ export function useLeaderboardLegendItems( { // Add comparison period legend item if comparison is enabled and overlay label is not enabled if ( withComparison && ! withOverlayLabel ) { - const resolvedSecondaryColor = resolveGroupColor( { + const { color: resolvedSecondaryColor } = getElementStyles( { index: 1, overrideColor: secondaryColor || leaderboardChartSettings.secondaryColor, } ); @@ -83,7 +83,7 @@ export function useLeaderboardLegendItems( { withComparison, legendLabels, leaderboardChartSettings, - resolveGroupColor, + getElementStyles, withOverlayLabel, ] ); } diff --git a/projects/js-packages/charts/src/components/leaderboard-chart/leaderboard-chart.tsx b/projects/js-packages/charts/src/components/leaderboard-chart/leaderboard-chart.tsx index 3c73ff86b6188..57ecc7d79953f 100644 --- a/projects/js-packages/charts/src/components/leaderboard-chart/leaderboard-chart.tsx +++ b/projects/js-packages/charts/src/components/leaderboard-chart/leaderboard-chart.tsx @@ -159,12 +159,12 @@ const LeaderboardChartInternal: FC< LeaderboardChartProps > = ( { secondaryColor: settingsSecondaryColor, deltaColors, } = leaderboardChartSettings; - const { resolveGroupColor } = useGlobalChartsContext(); - const resolvedPrimaryColor = resolveGroupColor( { + const { getElementStyles } = useGlobalChartsContext(); + const { color: resolvedPrimaryColor } = getElementStyles( { index: 0, overrideColor: primaryColor || settingsPrimaryColor, } ); - const resolvedSecondaryColor = resolveGroupColor( { + const { color: resolvedSecondaryColor } = getElementStyles( { index: 1, overrideColor: secondaryColor || settingsSecondaryColor, } ); diff --git a/projects/js-packages/charts/src/components/legend/private/base-legend.tsx b/projects/js-packages/charts/src/components/legend/private/base-legend.tsx index b9c8aceffb73a..ca9f708d7e018 100644 --- a/projects/js-packages/charts/src/components/legend/private/base-legend.tsx +++ b/projects/js-packages/charts/src/components/legend/private/base-legend.tsx @@ -91,24 +91,25 @@ export const BaseLegend: ForwardRefExoticComponent< ) => { const theme = useGlobalChartsTheme(); const context = useContext( GlobalChartsContext ); - const resolveGroupColor = context?.resolveGroupColor; + const getElementStyles = context?.getElementStyles; // Resolve colors dynamically for items that have group info const itemsWithResolvedColors = useMemo( () => { return items.map( item => { // If item has group info and we have a context, resolve color dynamically - if ( item.group !== undefined && item.index !== undefined && resolveGroupColor ) { - const resolvedColor = resolveGroupColor( { - group: item.group, + if ( item.group !== undefined && item.index !== undefined && getElementStyles ) { + const { color } = getElementStyles( { + data: item, index: item.index, overrideColor: item.overrideColor, } ); - return { ...item, color: resolvedColor }; + + return { ...item, color }; } // Otherwise use the static color return item; } ); - }, [ items, resolveGroupColor ] ); + }, [ items, getElementStyles ] ); const legendScale = scaleOrdinal( { domain: itemsWithResolvedColors.map( item => item.label ), diff --git a/projects/js-packages/charts/src/components/line-chart/line-chart.tsx b/projects/js-packages/charts/src/components/line-chart/line-chart.tsx index 751cc8f924418..a7da02a3f8e58 100644 --- a/projects/js-packages/charts/src/components/line-chart/line-chart.tsx +++ b/projects/js-packages/charts/src/components/line-chart/line-chart.tsx @@ -20,7 +20,7 @@ import { useGlobalChartsTheme, ChartTypes, } from '../../providers'; -import { attachSubComponents, getSeriesLineStyles } from '../../utils'; +import { attachSubComponents } from '../../utils'; import { Legend, useChartLegendItems } from '../legend'; import { DefaultGlyph } from '../private/default-glyph'; import { SingleChartContext, type SingleChartRef } from '../private/single-chart-context'; @@ -253,7 +253,7 @@ const LineChartInternal = forwardRef< SingleChartRef, LineChartProps >( ); const dataSorted = useChartDataTransform( data ); - const { resolveGroupColor } = useGlobalChartsContext(); + const { getElementStyles } = useGlobalChartsContext(); // Use the keyboard navigation hook const { tooltipRef, onChartFocus, onChartBlur, onChartKeyDown } = useKeyboardNavigation( { @@ -304,21 +304,18 @@ const LineChartInternal = forwardRef< SingleChartRef, LineChartProps >( // Resolve group color for tooltip glyph const seriesData = dataSorted[ seriesIndex ]; - const resolvedColor = seriesData - ? resolveGroupColor( { - group: seriesData.group, - index: seriesIndex, - overrideColor: seriesData.options?.stroke, - } ) + const color = seriesData + ? getElementStyles( { data: seriesData, index: seriesIndex, chartType: ChartTypes.Line } ) + .color : props.color; - const propsWithResolvedColor = { ...props, color: resolvedColor }; + const propsWithResolvedColor = { ...props, color }; const themeGlyph = providerTheme.glyphs?.[ seriesIndex ]; return themeGlyph ? themeGlyph( propsWithResolvedColor ) : renderGlyph( propsWithResolvedColor ); }; - }, [ dataSorted, providerTheme.glyphs, renderGlyph, resolveGroupColor ] ); + }, [ dataSorted, providerTheme.glyphs, renderGlyph, getElementStyles ] ); const defaultMargin = useChartMargin( height, chartOptions, dataSorted, theme ); @@ -423,15 +420,14 @@ const LineChartInternal = forwardRef< SingleChartRef, LineChartProps >( { dataSorted.map( ( seriesData, index ) => { - const stroke = resolveGroupColor( { - group: seriesData.group, + const { color, lineStyles } = getElementStyles( { + data: seriesData, index, - overrideColor: seriesData.options?.stroke, + chartType: ChartTypes.Line, } ); - const lineStyles = getSeriesLineStyles( seriesData, index, providerTheme ); const lineProps = { - stroke, + stroke: color, ...lineStyles, }; @@ -441,7 +437,7 @@ const LineChartInternal = forwardRef< SingleChartRef, LineChartProps >( ( { withGradientFill && ( d.value, // Use the color property from the data object as a last resort. The theme provides colours by default. - fill: ( { group, index, color: overrideColor }: DataPointPercentage & { index: number } ) => - resolveGroupColor( { group, index, overrideColor } ), + fill: ( d: DataPointPercentage & { index: number } ) => { + const { index, color: overrideColor } = d; + return getElementStyles( { data: d, index, overrideColor } ).color; + }, }; return ( diff --git a/projects/js-packages/charts/src/components/pie-semi-circle-chart/pie-semi-circle-chart.tsx b/projects/js-packages/charts/src/components/pie-semi-circle-chart/pie-semi-circle-chart.tsx index 31d67d514bbe1..679b3104eaf78 100644 --- a/projects/js-packages/charts/src/components/pie-semi-circle-chart/pie-semi-circle-chart.tsx +++ b/projects/js-packages/charts/src/components/pie-semi-circle-chart/pie-semi-circle-chart.tsx @@ -161,7 +161,7 @@ const PieSemiCircleChartInternal: FC< PieSemiCircleChartProps > = ( { // Validate data first to get validation result const { isValid, message } = validateData( data ); - const { resolveGroupColor } = useGlobalChartsContext(); + const { getElementStyles } = useGlobalChartsContext(); // Define accessors with useMemo to avoid changing dependencies const accessors = useMemo( @@ -171,10 +171,10 @@ const PieSemiCircleChartInternal: FC< PieSemiCircleChartProps > = ( { a: DataPointPercentage & { index: number }, b: DataPointPercentage & { index: number } ) => b.value - a.value, - fill: ( { group, index, color: overrideColor }: DataPointPercentage & { index: number } ) => - resolveGroupColor( { group, index, overrideColor } ), + fill: ( d: DataPointPercentage & { index: number } ) => + getElementStyles( { data: d, index: d.index, overrideColor: d.color } ).color, } ), - [ resolveGroupColor ] + [ getElementStyles ] ); // Memoize legend options to prevent unnecessary re-calculations diff --git a/projects/js-packages/charts/src/providers/chart-context/global-charts-provider.tsx b/projects/js-packages/charts/src/providers/chart-context/global-charts-provider.tsx index 49ddf88eea930..856871cf6c2e8 100644 --- a/projects/js-packages/charts/src/providers/chart-context/global-charts-provider.tsx +++ b/projects/js-packages/charts/src/providers/chart-context/global-charts-provider.tsx @@ -1,5 +1,5 @@ import { createContext, useCallback, useMemo, useState, useEffect, useRef } from 'react'; -import { mergeThemes } from '../../utils'; +import { getSeriesLineStyles, mergeThemes } from '../../utils'; import { defaultTheme } from './themes'; import type { GlobalChartsContextValue, ChartRegistration } from './types'; import type { ChartTheme, CompleteChartTheme } from '../../types'; @@ -51,7 +51,7 @@ export const GlobalChartsProvider: FC< GlobalChartsProviderProps > = ( { [ charts ] ); - const resolveGroupColor = useCallback< GlobalChartsContextValue[ 'resolveGroupColor' ] >( + const resolveColor = useCallback< GlobalChartsContextValue[ 'resolveColor' ] >( ( { group, index, overrideColor } ) => { // Highest precedence: explicit series stroke if ( overrideColor ) { @@ -80,6 +80,19 @@ export const GlobalChartsProvider: FC< GlobalChartsProviderProps > = ( { [ providerTheme.colors ] ); + const getElementStyles = useCallback< GlobalChartsContextValue[ 'getElementStyles' ] >( + ( { data, index, overrideColor } ) => { + return { + color: resolveColor( { group: data?.group, index, overrideColor } ), + lineStyles: + data && typeof data === 'object' && 'data' in data && 'options' in data + ? getSeriesLineStyles( data, index, providerTheme ) + : {}, + }; + }, + [ providerTheme, resolveColor ] + ); + const value: GlobalChartsContextValue = useMemo( () => ( { charts, @@ -87,9 +100,18 @@ export const GlobalChartsProvider: FC< GlobalChartsProviderProps > = ( { unregisterChart, getChartData, theme: providerTheme, - resolveGroupColor, + resolveColor, + getElementStyles, } ), - [ charts, registerChart, unregisterChart, getChartData, providerTheme, resolveGroupColor ] + [ + charts, + registerChart, + unregisterChart, + getChartData, + providerTheme, + resolveColor, + getElementStyles, + ] ); return { children }; diff --git a/projects/js-packages/charts/src/providers/chart-context/test/chart-context.test.tsx b/projects/js-packages/charts/src/providers/chart-context/test/chart-context.test.tsx index 4cd2b67bada6a..6b3b27f3dda69 100644 --- a/projects/js-packages/charts/src/providers/chart-context/test/chart-context.test.tsx +++ b/projects/js-packages/charts/src/providers/chart-context/test/chart-context.test.tsx @@ -218,7 +218,7 @@ describe( 'ChartContext', () => { } ); describe( 'Group Color Resolver', () => { - it( 'provides resolveGroupColor function', () => { + it( 'provides resolveColor function', () => { let contextValue: GlobalChartsContextValue; const TestComponent = () => { @@ -232,7 +232,7 @@ describe( 'ChartContext', () => { ); - expect( contextValue.resolveGroupColor ).toBeInstanceOf( Function ); + expect( contextValue.resolveColor ).toBeInstanceOf( Function ); } ); it( 'returns consistent colors for same group across different indices', () => { @@ -249,9 +249,9 @@ describe( 'ChartContext', () => { ); - const color1 = contextValue.resolveGroupColor( { group: 'united-states', index: 0 } ); - const color2 = contextValue.resolveGroupColor( { group: 'united-states', index: 5 } ); - const color3 = contextValue.resolveGroupColor( { group: 'united-states', index: 10 } ); + const color1 = contextValue.resolveColor( { group: 'united-states', index: 0 } ); + const color2 = contextValue.resolveColor( { group: 'united-states', index: 5 } ); + const color3 = contextValue.resolveColor( { group: 'united-states', index: 10 } ); expect( color1 ).toBe( color2 ); expect( color2 ).toBe( color3 ); @@ -271,9 +271,9 @@ describe( 'ChartContext', () => { ); - const usColor = contextValue.resolveGroupColor( { group: 'united-states', index: 0 } ); - const gbColor = contextValue.resolveGroupColor( { group: 'great-britain', index: 0 } ); - const jpColor = contextValue.resolveGroupColor( { group: 'japan', index: 0 } ); + const usColor = contextValue.resolveColor( { group: 'united-states', index: 0 } ); + const gbColor = contextValue.resolveColor( { group: 'great-britain', index: 0 } ); + const jpColor = contextValue.resolveColor( { group: 'japan', index: 0 } ); expect( usColor ).not.toBe( gbColor ); expect( gbColor ).not.toBe( jpColor ); @@ -295,12 +295,12 @@ describe( 'ChartContext', () => { ); const overrideColor = '#ff6600'; - const colorWithOverride = contextValue.resolveGroupColor( { + const colorWithOverride = contextValue.resolveColor( { group: 'united-states', index: 0, overrideColor, } ); - const colorWithoutOverride = contextValue.resolveGroupColor( { + const colorWithoutOverride = contextValue.resolveColor( { group: 'united-states', index: 0, } ); @@ -323,7 +323,7 @@ describe( 'ChartContext', () => { ); - const color = contextValue.resolveGroupColor( { group: undefined, index: 0 } ); + const color = contextValue.resolveColor( { group: undefined, index: 0 } ); expect( color ).toBe( mockTheme.colors[ 0 ] ); } ); @@ -342,7 +342,7 @@ describe( 'ChartContext', () => { ); - const color = contextValue.resolveGroupColor( { group: '', index: 0 } ); + const color = contextValue.resolveColor( { group: '', index: 0 } ); expect( color ).toBe( mockTheme.colors[ 0 ] ); } ); @@ -361,9 +361,9 @@ describe( 'ChartContext', () => { ); - const color1 = contextValue.resolveGroupColor( { group: undefined, index: 0 } ); - const color2 = contextValue.resolveGroupColor( { group: '', index: 1 } ); - const color3 = contextValue.resolveGroupColor( { + const color1 = contextValue.resolveColor( { group: undefined, index: 0 } ); + const color2 = contextValue.resolveColor( { group: '', index: 1 } ); + const color3 = contextValue.resolveColor( { group: null as string | undefined, index: 2, } ); @@ -388,8 +388,8 @@ describe( 'ChartContext', () => { ); // mockTheme has 3 colors, so index 3 should wrap to index 0 - const color1 = contextValue.resolveGroupColor( { group: undefined, index: 3 } ); - const color2 = contextValue.resolveGroupColor( { group: undefined, index: 0 } ); + const color1 = contextValue.resolveColor( { group: undefined, index: 3 } ); + const color2 = contextValue.resolveColor( { group: undefined, index: 0 } ); expect( color1 ).toBe( color2 ); expect( color1 ).toBe( mockTheme.colors[ 0 ] ); @@ -412,9 +412,9 @@ describe( 'ChartContext', () => { const groupName = 'consistent-group'; const colors = []; - // Call resolveGroupColor multiple times for the same group + // Call resolveColor multiple times for the same group for ( let i = 0; i < 10; i++ ) { - colors.push( contextValue.resolveGroupColor( { group: groupName, index: i } ) ); + colors.push( contextValue.resolveColor( { group: groupName, index: i } ) ); } // All colors should be the same @@ -441,8 +441,8 @@ describe( 'ChartContext', () => { const groupName = 'test-group'; const overrideColor = '#purple'; - const groupColor = contextValue.resolveGroupColor( { group: groupName, index: 0 } ); - const overriddenColor = contextValue.resolveGroupColor( { + const groupColor = contextValue.resolveColor( { group: groupName, index: 0 } ); + const overriddenColor = contextValue.resolveColor( { group: groupName, index: 0, overrideColor, @@ -475,7 +475,7 @@ describe( 'ChartContext', () => { // Get initial colors for all groups const initialColors = initialGroups.map( ( { group, index } ) => - contextValue.resolveGroupColor( { group, index } ) + contextValue.resolveColor( { group, index } ) ); // Simulate removing the middle group (great-britain) @@ -487,7 +487,7 @@ describe( 'ChartContext', () => { // Get colors after "filtering" const filteredColors = filteredGroups.map( ( { group, index } ) => - contextValue.resolveGroupColor( { group, index } ) + contextValue.resolveColor( { group, index } ) ); // Colors should remain the same despite index changes @@ -513,18 +513,18 @@ describe( 'ChartContext', () => { ); // Get initial colors for all groups - const usColor1 = contextValue.resolveGroupColor( { group: 'united-states', index: 0 } ); - const gbColor1 = contextValue.resolveGroupColor( { group: 'great-britain', index: 1 } ); - const jpColor1 = contextValue.resolveGroupColor( { group: 'japan', index: 2 } ); + const usColor1 = contextValue.resolveColor( { group: 'united-states', index: 0 } ); + const gbColor1 = contextValue.resolveColor( { group: 'great-britain', index: 1 } ); + const jpColor1 = contextValue.resolveColor( { group: 'japan', index: 2 } ); // Simulate removing great-britain (only US and Japan visible) - const usColor2 = contextValue.resolveGroupColor( { group: 'united-states', index: 0 } ); - const jpColor2 = contextValue.resolveGroupColor( { group: 'japan', index: 1 } ); + const usColor2 = contextValue.resolveColor( { group: 'united-states', index: 0 } ); + const jpColor2 = contextValue.resolveColor( { group: 'japan', index: 1 } ); // Simulate re-adding great-britain back (all groups visible again) - const usColor3 = contextValue.resolveGroupColor( { group: 'united-states', index: 0 } ); - const gbColor3 = contextValue.resolveGroupColor( { group: 'great-britain', index: 1 } ); - const jpColor3 = contextValue.resolveGroupColor( { group: 'japan', index: 2 } ); + const usColor3 = contextValue.resolveColor( { group: 'united-states', index: 0 } ); + const gbColor3 = contextValue.resolveColor( { group: 'great-britain', index: 1 } ); + const jpColor3 = contextValue.resolveColor( { group: 'japan', index: 2 } ); // All colors should remain stable throughout the process expect( usColor1 ).toBe( usColor2 ); @@ -548,7 +548,7 @@ describe( 'ChartContext', () => { registerChart: GlobalChartsContextValue[ 'registerChart' ]; unregisterChart: GlobalChartsContextValue[ 'unregisterChart' ]; getChartData: GlobalChartsContextValue[ 'getChartData' ]; - resolveGroupColor: GlobalChartsContextValue[ 'resolveGroupColor' ]; + resolveColor: GlobalChartsContextValue[ 'resolveColor' ]; } > = []; const TestComponent = () => { @@ -557,7 +557,7 @@ describe( 'ChartContext', () => { registerChart: context.registerChart, unregisterChart: context.unregisterChart, getChartData: context.getChartData, - resolveGroupColor: context.resolveGroupColor, + resolveColor: context.resolveColor, } ); return
Test
; }; @@ -578,7 +578,7 @@ describe( 'ChartContext', () => { expect( functionRefs[ 0 ].registerChart ).toBe( functionRefs[ 1 ].registerChart ); expect( functionRefs[ 0 ].unregisterChart ).toBe( functionRefs[ 1 ].unregisterChart ); expect( functionRefs[ 0 ].getChartData ).toBe( functionRefs[ 1 ].getChartData ); - expect( functionRefs[ 0 ].resolveGroupColor ).toBe( functionRefs[ 1 ].resolveGroupColor ); + expect( functionRefs[ 0 ].resolveColor ).toBe( functionRefs[ 1 ].resolveColor ); } ); } ); } ); diff --git a/projects/js-packages/charts/src/providers/chart-context/types.ts b/projects/js-packages/charts/src/providers/chart-context/types.ts index 3f9c4f16c2426..d85bea0970866 100644 --- a/projects/js-packages/charts/src/providers/chart-context/types.ts +++ b/projects/js-packages/charts/src/providers/chart-context/types.ts @@ -1,7 +1,9 @@ import type { ChartTypes } from './constants'; import type { BaseLegendItem } from '../../components/legend'; -import type { CompleteChartTheme } from '../../types'; -export type ChartType = keyof typeof ChartTypes; +import type { CompleteChartTheme, DataPointPercentage, SeriesData } from '../../types'; +import type { LineStyles } from '@visx/xychart'; + +export type ChartType = ( typeof ChartTypes )[ keyof typeof ChartTypes ]; export interface ChartRegistration { legendItems: BaseLegendItem[]; @@ -9,6 +11,18 @@ export interface ChartRegistration { metadata?: Record< string, unknown >; } +export type GetElementStylesParams = { + index: number; + chartType?: ChartType; + data?: SeriesData | DataPointPercentage | BaseLegendItem; + overrideColor?: string; +}; + +export type ElementStyles = { + color: string; + lineStyles: LineStyles; +}; + export interface GlobalChartsContextValue { charts: Map< string, ChartRegistration >; registerChart: ( id: string, data: ChartRegistration ) => void; @@ -22,9 +36,9 @@ export interface GlobalChartsContextValue { * - If a group is provided, returns a stable color per group across charts. * - If no group, falls back to index-based color from the theme palette. */ - resolveGroupColor: ( params: { - group?: string; - index: number; - overrideColor?: string; - } ) => string; + resolveColor: ( params: { group?: string; index: number; overrideColor?: string } ) => string; + /** + * Get the styles for a series. + */ + getElementStyles: ( params: GetElementStylesParams ) => ElementStyles; } From f0db3cd3d9199c3f70be38b99a95ac3cda85b823 Mon Sep 17 00:00:00 2001 From: Adam Wood <1017872+adamwoodnz@users.noreply.github.com> Date: Wed, 17 Sep 2025 16:54:39 +1200 Subject: [PATCH 03/23] Add changelog --- ...-charts-add-hook-to-get-the-styles-for-any-chart-component | 4 ++++ 1 file changed, 4 insertions(+) create mode 100644 projects/js-packages/charts/changelog/charts-111-charts-add-hook-to-get-the-styles-for-any-chart-component diff --git a/projects/js-packages/charts/changelog/charts-111-charts-add-hook-to-get-the-styles-for-any-chart-component b/projects/js-packages/charts/changelog/charts-111-charts-add-hook-to-get-the-styles-for-any-chart-component new file mode 100644 index 0000000000000..b1f6d850e80be --- /dev/null +++ b/projects/js-packages/charts/changelog/charts-111-charts-add-hook-to-get-the-styles-for-any-chart-component @@ -0,0 +1,4 @@ +Significance: minor +Type: added + +Charts: Add get element styles utility to global context From 01ed5cc8ff760ffd11da25ba7a574dac6548bcf0 Mon Sep 17 00:00:00 2001 From: Adam Wood <1017872+adamwoodnz@users.noreply.github.com> Date: Wed, 17 Sep 2025 17:03:34 +1200 Subject: [PATCH 04/23] Revert adding chartType --- .../charts/src/components/bar-chart/bar-chart.tsx | 7 +++---- .../charts/src/components/line-chart/line-chart.tsx | 4 +--- .../charts/src/providers/chart-context/types.ts | 1 - 3 files changed, 4 insertions(+), 8 deletions(-) diff --git a/projects/js-packages/charts/src/components/bar-chart/bar-chart.tsx b/projects/js-packages/charts/src/components/bar-chart/bar-chart.tsx index f4144c72a3ad6..f4fcd227e5383 100644 --- a/projects/js-packages/charts/src/components/bar-chart/bar-chart.tsx +++ b/projects/js-packages/charts/src/components/bar-chart/bar-chart.tsx @@ -130,11 +130,10 @@ const BarChartInternal: FC< BarChartProps > = ( { const { getElementStyles } = useGlobalChartsContext(); const getBarBackground = useCallback( - ( index: number ) => () => { - return withPatterns + ( index: number ) => () => + withPatterns ? `url(#${ getPatternId( chartId, index ) })` - : getElementStyles( { data: dataSorted[ index ], index, chartType: ChartTypes.Bar } ).color; - }, + : getElementStyles( { data: dataSorted[ index ], index } ).color, [ withPatterns, getElementStyles, dataSorted, chartId ] ); diff --git a/projects/js-packages/charts/src/components/line-chart/line-chart.tsx b/projects/js-packages/charts/src/components/line-chart/line-chart.tsx index a7da02a3f8e58..a0f99a0fa8b5e 100644 --- a/projects/js-packages/charts/src/components/line-chart/line-chart.tsx +++ b/projects/js-packages/charts/src/components/line-chart/line-chart.tsx @@ -305,8 +305,7 @@ const LineChartInternal = forwardRef< SingleChartRef, LineChartProps >( // Resolve group color for tooltip glyph const seriesData = dataSorted[ seriesIndex ]; const color = seriesData - ? getElementStyles( { data: seriesData, index: seriesIndex, chartType: ChartTypes.Line } ) - .color + ? getElementStyles( { data: seriesData, index: seriesIndex } ).color : props.color; const propsWithResolvedColor = { ...props, color }; @@ -423,7 +422,6 @@ const LineChartInternal = forwardRef< SingleChartRef, LineChartProps >( const { color, lineStyles } = getElementStyles( { data: seriesData, index, - chartType: ChartTypes.Line, } ); const lineProps = { diff --git a/projects/js-packages/charts/src/providers/chart-context/types.ts b/projects/js-packages/charts/src/providers/chart-context/types.ts index d85bea0970866..2e79d65147f4d 100644 --- a/projects/js-packages/charts/src/providers/chart-context/types.ts +++ b/projects/js-packages/charts/src/providers/chart-context/types.ts @@ -13,7 +13,6 @@ export interface ChartRegistration { export type GetElementStylesParams = { index: number; - chartType?: ChartType; data?: SeriesData | DataPointPercentage | BaseLegendItem; overrideColor?: string; }; From 8bcfbf43d22c91987f849fb3d232b03110dac069 Mon Sep 17 00:00:00 2001 From: Adam Wood <1017872+adamwoodnz@users.noreply.github.com> Date: Wed, 17 Sep 2025 17:07:46 +1200 Subject: [PATCH 05/23] Revert adding chart type constant --- .../src/components/bar-chart/bar-chart.tsx | 3 +-- .../leaderboard-chart/leaderboard-chart.tsx | 3 +-- .../src/components/line-chart/line-chart.tsx | 3 +-- .../src/components/pie-chart/pie-chart.tsx | 3 +-- .../pie-semi-circle-chart.tsx | 3 +-- .../src/providers/chart-context/constants.ts | 10 ---------- .../hooks/use-chart-registration.ts | 3 +-- .../src/providers/chart-context/index.ts | 3 +-- .../chart-context/test/chart-context.test.tsx | 20 +++++++++---------- .../src/providers/chart-context/types.ts | 5 +---- 10 files changed, 18 insertions(+), 38 deletions(-) delete mode 100644 projects/js-packages/charts/src/providers/chart-context/constants.ts diff --git a/projects/js-packages/charts/src/components/bar-chart/bar-chart.tsx b/projects/js-packages/charts/src/components/bar-chart/bar-chart.tsx index f4fcd227e5383..282e2bdc14989 100644 --- a/projects/js-packages/charts/src/components/bar-chart/bar-chart.tsx +++ b/projects/js-packages/charts/src/components/bar-chart/bar-chart.tsx @@ -16,7 +16,6 @@ import { useChartRegistration, useGlobalChartsContext, GlobalChartsContext, - ChartTypes, } from '../../providers'; import { attachSubComponents } from '../../utils'; import { Legend, useChartLegendItems } from '../legend'; @@ -268,7 +267,7 @@ const BarChartInternal: FC< BarChartProps > = ( { useChartRegistration( { chartId, legendItems, - chartType: ChartTypes.Bar, + chartType: 'bar', isDataValid, metadata: chartMetadata, } ); diff --git a/projects/js-packages/charts/src/components/leaderboard-chart/leaderboard-chart.tsx b/projects/js-packages/charts/src/components/leaderboard-chart/leaderboard-chart.tsx index 57ecc7d79953f..831a31bfe678b 100644 --- a/projects/js-packages/charts/src/components/leaderboard-chart/leaderboard-chart.tsx +++ b/projects/js-packages/charts/src/components/leaderboard-chart/leaderboard-chart.tsx @@ -15,7 +15,6 @@ import { useChartRegistration, useGlobalChartsContext, useGlobalChartsTheme, - ChartTypes, } from '../../providers'; import { formatMetricValue, attachSubComponents } from '../../utils'; import { Legend } from '../legend'; @@ -195,7 +194,7 @@ const LeaderboardChartInternal: FC< LeaderboardChartProps > = ( { useChartRegistration( { chartId, legendItems, - chartType: ChartTypes.Leaderboard, + chartType: 'leaderboard', isDataValid, metadata: chartMetadata, } ); diff --git a/projects/js-packages/charts/src/components/line-chart/line-chart.tsx b/projects/js-packages/charts/src/components/line-chart/line-chart.tsx index a0f99a0fa8b5e..27b32050084fe 100644 --- a/projects/js-packages/charts/src/components/line-chart/line-chart.tsx +++ b/projects/js-packages/charts/src/components/line-chart/line-chart.tsx @@ -18,7 +18,6 @@ import { useChartRegistration, useGlobalChartsContext, useGlobalChartsTheme, - ChartTypes, } from '../../providers'; import { attachSubComponents } from '../../utils'; import { Legend, useChartLegendItems } from '../legend'; @@ -350,7 +349,7 @@ const LineChartInternal = forwardRef< SingleChartRef, LineChartProps >( useChartRegistration( { chartId, legendItems, - chartType: ChartTypes.Line, + chartType: 'line', isDataValid, metadata: chartMetadata, } ); diff --git a/projects/js-packages/charts/src/components/pie-chart/pie-chart.tsx b/projects/js-packages/charts/src/components/pie-chart/pie-chart.tsx index e12096516397d..0161c6e0ba99a 100644 --- a/projects/js-packages/charts/src/components/pie-chart/pie-chart.tsx +++ b/projects/js-packages/charts/src/components/pie-chart/pie-chart.tsx @@ -10,7 +10,6 @@ import { useGlobalChartsContext, useGlobalChartsTheme, GlobalChartsContext, - ChartTypes, } from '../../providers'; import { attachSubComponents } from '../../utils'; import { getStringWidth } from '../../visx/text'; @@ -173,7 +172,7 @@ const PieChartInternal = ( { useChartRegistration( { chartId, legendItems, - chartType: ChartTypes.Pie, + chartType: 'pie', isDataValid: isValid, metadata: chartMetadata, } ); diff --git a/projects/js-packages/charts/src/components/pie-semi-circle-chart/pie-semi-circle-chart.tsx b/projects/js-packages/charts/src/components/pie-semi-circle-chart/pie-semi-circle-chart.tsx index 679b3104eaf78..8e8324119ce5c 100644 --- a/projects/js-packages/charts/src/components/pie-semi-circle-chart/pie-semi-circle-chart.tsx +++ b/projects/js-packages/charts/src/components/pie-semi-circle-chart/pie-semi-circle-chart.tsx @@ -12,7 +12,6 @@ import { useChartRegistration, useGlobalChartsContext, GlobalChartsContext, - ChartTypes, } from '../../providers'; import { attachSubComponents } from '../../utils'; import { Legend, useChartLegendItems } from '../legend'; @@ -205,7 +204,7 @@ const PieSemiCircleChartInternal: FC< PieSemiCircleChartProps > = ( { useChartRegistration( { chartId, legendItems, - chartType: ChartTypes.PieSemiCircle, + chartType: 'pie-semi-circle', isDataValid: isValid, metadata: chartMetadata, } ); diff --git a/projects/js-packages/charts/src/providers/chart-context/constants.ts b/projects/js-packages/charts/src/providers/chart-context/constants.ts deleted file mode 100644 index 03fce921f9ab4..0000000000000 --- a/projects/js-packages/charts/src/providers/chart-context/constants.ts +++ /dev/null @@ -1,10 +0,0 @@ -/** - * Constants for all supported chart types in the charts package - */ -export const ChartTypes = { - Bar: 'bar', - Leaderboard: 'leaderboard', - Line: 'line', - Pie: 'pie', - PieSemiCircle: 'pie-semi-circle', -} as const; diff --git a/projects/js-packages/charts/src/providers/chart-context/hooks/use-chart-registration.ts b/projects/js-packages/charts/src/providers/chart-context/hooks/use-chart-registration.ts index 9148e27326f4b..52786a4fddd0d 100644 --- a/projects/js-packages/charts/src/providers/chart-context/hooks/use-chart-registration.ts +++ b/projects/js-packages/charts/src/providers/chart-context/hooks/use-chart-registration.ts @@ -2,7 +2,6 @@ import { useEffect, useMemo } from 'react'; import { useDeepMemo } from '../../../hooks'; import { useGlobalChartsContext } from './use-global-charts-context'; import type { BaseLegendItem } from '../../../components/legend'; -import type { ChartType } from '../../../types'; export const useChartRegistration = ( { chartId, @@ -13,7 +12,7 @@ export const useChartRegistration = ( { }: { chartId: string; legendItems: BaseLegendItem[]; - chartType: ChartType; + chartType: string; isDataValid: boolean; metadata?: Record< string, unknown >; } ): void => { diff --git a/projects/js-packages/charts/src/providers/chart-context/index.ts b/projects/js-packages/charts/src/providers/chart-context/index.ts index 55cf7b4c3dd9e..bd89ef2f46737 100644 --- a/projects/js-packages/charts/src/providers/chart-context/index.ts +++ b/projects/js-packages/charts/src/providers/chart-context/index.ts @@ -3,6 +3,5 @@ export { useGlobalChartsContext } from './hooks/use-global-charts-context'; export { useChartId } from './hooks/use-chart-id'; export { useChartRegistration } from './hooks/use-chart-registration'; export { useGlobalChartsTheme } from './hooks/use-global-charts-theme'; -export { ChartTypes } from './constants'; -export type { GlobalChartsContextValue, ChartRegistration, ChartType } from './types'; +export type { GlobalChartsContextValue, ChartRegistration } from './types'; export * from './themes'; diff --git a/projects/js-packages/charts/src/providers/chart-context/test/chart-context.test.tsx b/projects/js-packages/charts/src/providers/chart-context/test/chart-context.test.tsx index 6b3b27f3dda69..1f3854b5df239 100644 --- a/projects/js-packages/charts/src/providers/chart-context/test/chart-context.test.tsx +++ b/projects/js-packages/charts/src/providers/chart-context/test/chart-context.test.tsx @@ -1,6 +1,6 @@ import { render } from '@testing-library/react'; import { useMemo } from 'react'; -import { GlobalChartsProvider, ChartTypes } from '../global-charts-provider'; +import { GlobalChartsProvider } from '../global-charts-provider'; import { useChartId } from '../hooks/use-chart-id'; import { useChartRegistration } from '../hooks/use-chart-registration'; import { useGlobalChartsContext } from '../hooks/use-global-charts-context'; @@ -107,7 +107,7 @@ describe( 'ChartContext', () => { useChartRegistration( { chartId, legendItems: mockLegendItems, - chartType: ChartTypes.Bar, + chartType: 'bar', isDataValid: true, metadata, } ); @@ -124,7 +124,7 @@ describe( 'ChartContext', () => { const chartData = contextValue.getChartData( 'test-chart' ); expect( chartData ).toEqual( { legendItems: mockLegendItems, - chartType: ChartTypes.Bar, + chartType: 'bar', metadata: { test: true }, } ); } ); @@ -140,13 +140,13 @@ describe( 'ChartContext', () => { useChartRegistration( { chartId: chartId1, legendItems: mockLegendItems, - chartType: ChartTypes.Bar, + chartType: 'bar', isDataValid: true, } ); useChartRegistration( { chartId: chartId2, legendItems: mockLegendItems, - chartType: ChartTypes.Line, + chartType: 'line', isDataValid: true, } ); @@ -160,8 +160,8 @@ describe( 'ChartContext', () => { ); expect( contextValue.charts.size ).toBe( 2 ); - expect( contextValue.getChartData( 'chart-1' )?.chartType ).toBe( ChartTypes.Bar ); - expect( contextValue.getChartData( 'chart-2' )?.chartType ).toBe( ChartTypes.Line ); + expect( contextValue.getChartData( 'chart-1' )?.chartType ).toBe( 'bar' ); + expect( contextValue.getChartData( 'chart-2' )?.chartType ).toBe( 'line' ); } ); it( 'returns undefined for non-existent charts', () => { @@ -192,14 +192,14 @@ describe( 'ChartContext', () => { useChartRegistration( { chartId, legendItems: mockLegendItems, - chartType: ChartTypes.Bar, + chartType: 'bar', isDataValid: true, } ); // Register second chart with same ID useChartRegistration( { chartId, legendItems: mockLegendItems, - chartType: ChartTypes.Line, + chartType: 'line', isDataValid: true, } ); @@ -213,7 +213,7 @@ describe( 'ChartContext', () => { ); expect( contextValue.charts.size ).toBe( 1 ); - expect( contextValue.getChartData( 'same-id' )?.chartType ).toBe( ChartTypes.Line ); + expect( contextValue.getChartData( 'same-id' )?.chartType ).toBe( 'line' ); } ); } ); diff --git a/projects/js-packages/charts/src/providers/chart-context/types.ts b/projects/js-packages/charts/src/providers/chart-context/types.ts index 2e79d65147f4d..6edc025db37d2 100644 --- a/projects/js-packages/charts/src/providers/chart-context/types.ts +++ b/projects/js-packages/charts/src/providers/chart-context/types.ts @@ -1,13 +1,10 @@ -import type { ChartTypes } from './constants'; import type { BaseLegendItem } from '../../components/legend'; import type { CompleteChartTheme, DataPointPercentage, SeriesData } from '../../types'; import type { LineStyles } from '@visx/xychart'; -export type ChartType = ( typeof ChartTypes )[ keyof typeof ChartTypes ]; - export interface ChartRegistration { legendItems: BaseLegendItem[]; - chartType: ChartType; + chartType: string; metadata?: Record< string, unknown >; } From cc34adc18aeaa3de329cf96ee9736a7374413bed Mon Sep 17 00:00:00 2001 From: Adam Wood <1017872+adamwoodnz@users.noreply.github.com> Date: Wed, 17 Sep 2025 17:17:03 +1200 Subject: [PATCH 06/23] Remove resolveGroupColor from context value --- .../chart-context/global-charts-provider.tsx | 23 +++++++++---------- .../src/providers/chart-context/types.ts | 7 ------ 2 files changed, 11 insertions(+), 19 deletions(-) diff --git a/projects/js-packages/charts/src/providers/chart-context/global-charts-provider.tsx b/projects/js-packages/charts/src/providers/chart-context/global-charts-provider.tsx index 856871cf6c2e8..ae2219cebc97f 100644 --- a/projects/js-packages/charts/src/providers/chart-context/global-charts-provider.tsx +++ b/projects/js-packages/charts/src/providers/chart-context/global-charts-provider.tsx @@ -51,8 +51,16 @@ export const GlobalChartsProvider: FC< GlobalChartsProviderProps > = ( { [ charts ] ); - const resolveColor = useCallback< GlobalChartsContextValue[ 'resolveColor' ] >( - ( { group, index, overrideColor } ) => { + const resolveColor = useCallback( + ( { + group, + index, + overrideColor, + }: { + group?: string; + index: number; + overrideColor?: string; + } ): string => { // Highest precedence: explicit series stroke if ( overrideColor ) { return overrideColor; @@ -100,18 +108,9 @@ export const GlobalChartsProvider: FC< GlobalChartsProviderProps > = ( { unregisterChart, getChartData, theme: providerTheme, - resolveColor, getElementStyles, } ), - [ - charts, - registerChart, - unregisterChart, - getChartData, - providerTheme, - resolveColor, - getElementStyles, - ] + [ charts, registerChart, unregisterChart, getChartData, providerTheme, getElementStyles ] ); return { children }; diff --git a/projects/js-packages/charts/src/providers/chart-context/types.ts b/projects/js-packages/charts/src/providers/chart-context/types.ts index 6edc025db37d2..7e4878c54f007 100644 --- a/projects/js-packages/charts/src/providers/chart-context/types.ts +++ b/projects/js-packages/charts/src/providers/chart-context/types.ts @@ -26,13 +26,6 @@ export interface GlobalChartsContextValue { getChartData: ( id: string ) => ChartRegistration | undefined; /** Theme provided by the GlobalChartsProvider (merged with defaults) */ theme: CompleteChartTheme; - /** - * Resolve a stable color for a series. - * - If an override color is passed, it wins. - * - If a group is provided, returns a stable color per group across charts. - * - If no group, falls back to index-based color from the theme palette. - */ - resolveColor: ( params: { group?: string; index: number; overrideColor?: string } ) => string; /** * Get the styles for a series. */ From f26958bec2335083931dddd2aef8a123283c05bf Mon Sep 17 00:00:00 2001 From: Adam Wood <1017872+adamwoodnz@users.noreply.github.com> Date: Wed, 17 Sep 2025 17:35:51 +1200 Subject: [PATCH 07/23] Allow for data options stroke color --- .../chart-context/global-charts-provider.tsx | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/projects/js-packages/charts/src/providers/chart-context/global-charts-provider.tsx b/projects/js-packages/charts/src/providers/chart-context/global-charts-provider.tsx index ae2219cebc97f..ec3f8069dffbf 100644 --- a/projects/js-packages/charts/src/providers/chart-context/global-charts-provider.tsx +++ b/projects/js-packages/charts/src/providers/chart-context/global-charts-provider.tsx @@ -90,12 +90,15 @@ export const GlobalChartsProvider: FC< GlobalChartsProviderProps > = ( { const getElementStyles = useCallback< GlobalChartsContextValue[ 'getElementStyles' ] >( ( { data, index, overrideColor } ) => { + const isSeriesData = data && typeof data === 'object' && 'data' in data && 'options' in data; + return { - color: resolveColor( { group: data?.group, index, overrideColor } ), - lineStyles: - data && typeof data === 'object' && 'data' in data && 'options' in data - ? getSeriesLineStyles( data, index, providerTheme ) - : {}, + color: resolveColor( { + group: data?.group, + index, + overrideColor: overrideColor || ( isSeriesData && data?.options?.stroke ), + } ), + lineStyles: isSeriesData ? getSeriesLineStyles( data, index, providerTheme ) : {}, }; }, [ providerTheme, resolveColor ] From 648a728836f04c71162b3d1e3d71cd63d876ffcf Mon Sep 17 00:00:00 2001 From: Adam Wood <1017872+adamwoodnz@users.noreply.github.com> Date: Wed, 17 Sep 2025 17:43:03 +1200 Subject: [PATCH 08/23] Add glyph --- .../charts/src/components/line-chart/line-chart.tsx | 13 +++++++------ .../chart-context/global-charts-provider.tsx | 1 + .../charts/src/providers/chart-context/types.ts | 4 +++- 3 files changed, 11 insertions(+), 7 deletions(-) diff --git a/projects/js-packages/charts/src/components/line-chart/line-chart.tsx b/projects/js-packages/charts/src/components/line-chart/line-chart.tsx index 27b32050084fe..507043ad5d4cd 100644 --- a/projects/js-packages/charts/src/components/line-chart/line-chart.tsx +++ b/projects/js-packages/charts/src/components/line-chart/line-chart.tsx @@ -301,19 +301,20 @@ const LineChartInternal = forwardRef< SingleChartRef, LineChartProps >( series.label === props.key || series.data.includes( props.datum as DataPointDate ) ); - // Resolve group color for tooltip glyph const seriesData = dataSorted[ seriesIndex ]; - const color = seriesData - ? getElementStyles( { data: seriesData, index: seriesIndex } ).color - : props.color; + + const { color, glyph: themeGlyph } = getElementStyles( { + data: seriesData, + index: seriesIndex, + } ); const propsWithResolvedColor = { ...props, color }; - const themeGlyph = providerTheme.glyphs?.[ seriesIndex ]; + return themeGlyph ? themeGlyph( propsWithResolvedColor ) : renderGlyph( propsWithResolvedColor ); }; - }, [ dataSorted, providerTheme.glyphs, renderGlyph, getElementStyles ] ); + }, [ dataSorted, renderGlyph, getElementStyles ] ); const defaultMargin = useChartMargin( height, chartOptions, dataSorted, theme ); diff --git a/projects/js-packages/charts/src/providers/chart-context/global-charts-provider.tsx b/projects/js-packages/charts/src/providers/chart-context/global-charts-provider.tsx index ec3f8069dffbf..54a74e99bc472 100644 --- a/projects/js-packages/charts/src/providers/chart-context/global-charts-provider.tsx +++ b/projects/js-packages/charts/src/providers/chart-context/global-charts-provider.tsx @@ -99,6 +99,7 @@ export const GlobalChartsProvider: FC< GlobalChartsProviderProps > = ( { overrideColor: overrideColor || ( isSeriesData && data?.options?.stroke ), } ), lineStyles: isSeriesData ? getSeriesLineStyles( data, index, providerTheme ) : {}, + glyph: providerTheme.glyphs?.[ index ], }; }, [ providerTheme, resolveColor ] diff --git a/projects/js-packages/charts/src/providers/chart-context/types.ts b/projects/js-packages/charts/src/providers/chart-context/types.ts index 7e4878c54f007..dc5a11e6f4b1a 100644 --- a/projects/js-packages/charts/src/providers/chart-context/types.ts +++ b/projects/js-packages/charts/src/providers/chart-context/types.ts @@ -1,6 +1,7 @@ +import { ReactNode } from 'react'; import type { BaseLegendItem } from '../../components/legend'; import type { CompleteChartTheme, DataPointPercentage, SeriesData } from '../../types'; -import type { LineStyles } from '@visx/xychart'; +import type { GlyphProps, LineStyles } from '@visx/xychart'; export interface ChartRegistration { legendItems: BaseLegendItem[]; @@ -17,6 +18,7 @@ export type GetElementStylesParams = { export type ElementStyles = { color: string; lineStyles: LineStyles; + glyph: < Datum extends object >( props: GlyphProps< Datum > ) => ReactNode; }; export interface GlobalChartsContextValue { From eb2ac14a39d8f1e711c50ecfb3f923aa12c96676 Mon Sep 17 00:00:00 2001 From: Adam Wood <1017872+adamwoodnz@users.noreply.github.com> Date: Wed, 17 Sep 2025 17:46:47 +1200 Subject: [PATCH 09/23] Simplify resolveColor --- .../providers/chart-context/global-charts-provider.tsx | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/projects/js-packages/charts/src/providers/chart-context/global-charts-provider.tsx b/projects/js-packages/charts/src/providers/chart-context/global-charts-provider.tsx index 54a74e99bc472..ebb27c7551c2f 100644 --- a/projects/js-packages/charts/src/providers/chart-context/global-charts-provider.tsx +++ b/projects/js-packages/charts/src/providers/chart-context/global-charts-provider.tsx @@ -66,7 +66,7 @@ export const GlobalChartsProvider: FC< GlobalChartsProviderProps > = ( { return overrideColor; } - const palette = providerTheme.colors ?? []; + const { colors } = providerTheme; // If group provided, maintain a stable assignment if ( group ) { @@ -77,15 +77,15 @@ export const GlobalChartsProvider: FC< GlobalChartsProviderProps > = ( { // Assign next color from palette in a deterministic cycling manner const assignedCount = groupToColorMapRef.current.size; - const color = palette.length > 0 ? palette[ assignedCount % palette.length ] : '#000000'; + const color = colors.length > 0 ? colors[ assignedCount % colors.length ] : '#000000'; groupToColorMapRef.current.set( group, color ); return color; } // Fallback: index-based color cycling - return palette.length > 0 ? palette[ ( index || 0 ) % palette.length ] : '#000000'; + return colors.length > 0 ? colors[ ( index || 0 ) % colors.length ] : '#000000'; }, - [ providerTheme.colors ] + [ providerTheme ] ); const getElementStyles = useCallback< GlobalChartsContextValue[ 'getElementStyles' ] >( From 6b1fe74643fe9feb020d3cb95ac14e77cba69597 Mon Sep 17 00:00:00 2001 From: Adam Wood <1017872+adamwoodnz@users.noreply.github.com> Date: Thu, 18 Sep 2025 09:43:53 +1200 Subject: [PATCH 10/23] Fix global context tests --- .../chart-context/global-charts-provider.tsx | 9 +- .../chart-context/test/chart-context.test.tsx | 173 +++++++++++++----- 2 files changed, 131 insertions(+), 51 deletions(-) diff --git a/projects/js-packages/charts/src/providers/chart-context/global-charts-provider.tsx b/projects/js-packages/charts/src/providers/chart-context/global-charts-provider.tsx index ebb27c7551c2f..7ad451a6e2406 100644 --- a/projects/js-packages/charts/src/providers/chart-context/global-charts-provider.tsx +++ b/projects/js-packages/charts/src/providers/chart-context/global-charts-provider.tsx @@ -13,14 +13,11 @@ export interface GlobalChartsProviderProps { theme?: Partial< ChartTheme >; } -export const GlobalChartsProvider: FC< GlobalChartsProviderProps > = ( { - children, - theme = {}, -} ) => { +export const GlobalChartsProvider: FC< GlobalChartsProviderProps > = ( { children, theme } ) => { const [ charts, setCharts ] = useState< Map< string, ChartRegistration > >( () => new Map() ); const providerTheme: CompleteChartTheme = useMemo( - () => mergeThemes( defaultTheme, theme ), + () => ( theme ? mergeThemes( defaultTheme, theme ) : defaultTheme ), [ theme ] ); @@ -61,7 +58,7 @@ export const GlobalChartsProvider: FC< GlobalChartsProviderProps > = ( { index: number; overrideColor?: string; } ): string => { - // Highest precedence: explicit series stroke + // Highest precedence: eg. explicit series stroke or chart color prop if ( overrideColor ) { return overrideColor; } diff --git a/projects/js-packages/charts/src/providers/chart-context/test/chart-context.test.tsx b/projects/js-packages/charts/src/providers/chart-context/test/chart-context.test.tsx index 1f3854b5df239..15a5e4e75f660 100644 --- a/projects/js-packages/charts/src/providers/chart-context/test/chart-context.test.tsx +++ b/projects/js-packages/charts/src/providers/chart-context/test/chart-context.test.tsx @@ -5,7 +5,7 @@ import { useChartId } from '../hooks/use-chart-id'; import { useChartRegistration } from '../hooks/use-chart-registration'; import { useGlobalChartsContext } from '../hooks/use-global-charts-context'; import type { BaseLegendItem } from '../../../components/legend'; -import type { ChartTheme } from '../../../types'; +import type { ChartTheme, SeriesData } from '../../../types'; import type { GlobalChartsContextValue } from '../types'; describe( 'ChartContext', () => { @@ -18,6 +18,13 @@ describe( 'ChartContext', () => { { label: 'Series 2', value: '200', color: '#00ff00' }, ]; + // Helper function to create mock data for color resolution tests + const createMockDataWithGroup = ( group: string | undefined ): SeriesData => ( { + label: 'Test', + data: [ { value: 100 } ], + group, + } ); + describe( 'GlobalChartsProvider', () => { it( 'provides context to child components', () => { let contextValue: GlobalChartsContextValue; @@ -218,7 +225,7 @@ describe( 'ChartContext', () => { } ); describe( 'Group Color Resolver', () => { - it( 'provides resolveColor function', () => { + it( 'provides getElementStyles function for color resolution', () => { let contextValue: GlobalChartsContextValue; const TestComponent = () => { @@ -232,7 +239,7 @@ describe( 'ChartContext', () => { ); - expect( contextValue.resolveColor ).toBeInstanceOf( Function ); + expect( contextValue.getElementStyles ).toBeInstanceOf( Function ); } ); it( 'returns consistent colors for same group across different indices', () => { @@ -249,9 +256,18 @@ describe( 'ChartContext', () => { ); - const color1 = contextValue.resolveColor( { group: 'united-states', index: 0 } ); - const color2 = contextValue.resolveColor( { group: 'united-states', index: 5 } ); - const color3 = contextValue.resolveColor( { group: 'united-states', index: 10 } ); + const color1 = contextValue.getElementStyles( { + data: createMockDataWithGroup( 'united-states' ), + index: 0, + } ).color; + const color2 = contextValue.getElementStyles( { + data: createMockDataWithGroup( 'united-states' ), + index: 5, + } ).color; + const color3 = contextValue.getElementStyles( { + data: createMockDataWithGroup( 'united-states' ), + index: 10, + } ).color; expect( color1 ).toBe( color2 ); expect( color2 ).toBe( color3 ); @@ -271,9 +287,18 @@ describe( 'ChartContext', () => { ); - const usColor = contextValue.resolveColor( { group: 'united-states', index: 0 } ); - const gbColor = contextValue.resolveColor( { group: 'great-britain', index: 0 } ); - const jpColor = contextValue.resolveColor( { group: 'japan', index: 0 } ); + const usColor = contextValue.getElementStyles( { + data: createMockDataWithGroup( 'united-states' ), + index: 0, + } ).color; + const gbColor = contextValue.getElementStyles( { + data: createMockDataWithGroup( 'great-britain' ), + index: 0, + } ).color; + const jpColor = contextValue.getElementStyles( { + data: createMockDataWithGroup( 'japan' ), + index: 0, + } ).color; expect( usColor ).not.toBe( gbColor ); expect( gbColor ).not.toBe( jpColor ); @@ -295,15 +320,15 @@ describe( 'ChartContext', () => { ); const overrideColor = '#ff6600'; - const colorWithOverride = contextValue.resolveColor( { - group: 'united-states', + const colorWithOverride = contextValue.getElementStyles( { + data: createMockDataWithGroup( 'united-states' ), index: 0, overrideColor, - } ); - const colorWithoutOverride = contextValue.resolveColor( { - group: 'united-states', + } ).color; + const colorWithoutOverride = contextValue.getElementStyles( { + data: createMockDataWithGroup( 'united-states' ), index: 0, - } ); + } ).color; expect( colorWithOverride ).toBe( overrideColor ); expect( colorWithoutOverride ).not.toBe( overrideColor ); @@ -323,7 +348,10 @@ describe( 'ChartContext', () => { ); - const color = contextValue.resolveColor( { group: undefined, index: 0 } ); + const color = contextValue.getElementStyles( { + data: createMockDataWithGroup( undefined ), + index: 0, + } ).color; expect( color ).toBe( mockTheme.colors[ 0 ] ); } ); @@ -342,7 +370,10 @@ describe( 'ChartContext', () => { ); - const color = contextValue.resolveColor( { group: '', index: 0 } ); + const color = contextValue.getElementStyles( { + data: createMockDataWithGroup( '' ), + index: 0, + } ).color; expect( color ).toBe( mockTheme.colors[ 0 ] ); } ); @@ -361,12 +392,18 @@ describe( 'ChartContext', () => { ); - const color1 = contextValue.resolveColor( { group: undefined, index: 0 } ); - const color2 = contextValue.resolveColor( { group: '', index: 1 } ); - const color3 = contextValue.resolveColor( { - group: null as string | undefined, + const color1 = contextValue.getElementStyles( { + data: createMockDataWithGroup( undefined ), + index: 0, + } ).color; + const color2 = contextValue.getElementStyles( { + data: createMockDataWithGroup( '' ), + index: 1, + } ).color; + const color3 = contextValue.getElementStyles( { + data: createMockDataWithGroup( null as string | undefined ), index: 2, - } ); + } ).color; expect( color1 ).toBe( mockTheme.colors[ 0 ] ); expect( color2 ).toBe( mockTheme.colors[ 1 ] ); @@ -388,8 +425,14 @@ describe( 'ChartContext', () => { ); // mockTheme has 3 colors, so index 3 should wrap to index 0 - const color1 = contextValue.resolveColor( { group: undefined, index: 3 } ); - const color2 = contextValue.resolveColor( { group: undefined, index: 0 } ); + const color1 = contextValue.getElementStyles( { + data: createMockDataWithGroup( undefined ), + index: 3, + } ).color; + const color2 = contextValue.getElementStyles( { + data: createMockDataWithGroup( undefined ), + index: 0, + } ).color; expect( color1 ).toBe( color2 ); expect( color1 ).toBe( mockTheme.colors[ 0 ] ); @@ -412,9 +455,14 @@ describe( 'ChartContext', () => { const groupName = 'consistent-group'; const colors = []; - // Call resolveColor multiple times for the same group + // Call getElementStyles multiple times for the same group for ( let i = 0; i < 10; i++ ) { - colors.push( contextValue.resolveColor( { group: groupName, index: i } ) ); + colors.push( + contextValue.getElementStyles( { + data: createMockDataWithGroup( groupName ), + index: i, + } ).color + ); } // All colors should be the same @@ -441,12 +489,15 @@ describe( 'ChartContext', () => { const groupName = 'test-group'; const overrideColor = '#purple'; - const groupColor = contextValue.resolveColor( { group: groupName, index: 0 } ); - const overriddenColor = contextValue.resolveColor( { - group: groupName, + const groupColor = contextValue.getElementStyles( { + data: createMockDataWithGroup( groupName ), + index: 0, + } ).color; + const overriddenColor = contextValue.getElementStyles( { + data: createMockDataWithGroup( groupName ), index: 0, overrideColor, - } ); + } ).color; expect( groupColor ).not.toBe( overrideColor ); expect( overriddenColor ).toBe( overrideColor ); @@ -474,8 +525,12 @@ describe( 'ChartContext', () => { ]; // Get initial colors for all groups - const initialColors = initialGroups.map( ( { group, index } ) => - contextValue.resolveColor( { group, index } ) + const initialColors = initialGroups.map( + ( { group, index } ) => + contextValue.getElementStyles( { + data: createMockDataWithGroup( group ), + index, + } ).color ); // Simulate removing the middle group (great-britain) @@ -486,8 +541,12 @@ describe( 'ChartContext', () => { ]; // Get colors after "filtering" - const filteredColors = filteredGroups.map( ( { group, index } ) => - contextValue.resolveColor( { group, index } ) + const filteredColors = filteredGroups.map( + ( { group, index } ) => + contextValue.getElementStyles( { + data: createMockDataWithGroup( group ), + index, + } ).color ); // Colors should remain the same despite index changes @@ -513,18 +572,42 @@ describe( 'ChartContext', () => { ); // Get initial colors for all groups - const usColor1 = contextValue.resolveColor( { group: 'united-states', index: 0 } ); - const gbColor1 = contextValue.resolveColor( { group: 'great-britain', index: 1 } ); - const jpColor1 = contextValue.resolveColor( { group: 'japan', index: 2 } ); + const usColor1 = contextValue.getElementStyles( { + data: createMockDataWithGroup( 'united-states' ), + index: 0, + } ).color; + const gbColor1 = contextValue.getElementStyles( { + data: createMockDataWithGroup( 'great-britain' ), + index: 1, + } ).color; + const jpColor1 = contextValue.getElementStyles( { + data: createMockDataWithGroup( 'japan' ), + index: 2, + } ).color; // Simulate removing great-britain (only US and Japan visible) - const usColor2 = contextValue.resolveColor( { group: 'united-states', index: 0 } ); - const jpColor2 = contextValue.resolveColor( { group: 'japan', index: 1 } ); + const usColor2 = contextValue.getElementStyles( { + data: createMockDataWithGroup( 'united-states' ), + index: 0, + } ).color; + const jpColor2 = contextValue.getElementStyles( { + data: createMockDataWithGroup( 'japan' ), + index: 1, + } ).color; // Simulate re-adding great-britain back (all groups visible again) - const usColor3 = contextValue.resolveColor( { group: 'united-states', index: 0 } ); - const gbColor3 = contextValue.resolveColor( { group: 'great-britain', index: 1 } ); - const jpColor3 = contextValue.resolveColor( { group: 'japan', index: 2 } ); + const usColor3 = contextValue.getElementStyles( { + data: createMockDataWithGroup( 'united-states' ), + index: 0, + } ).color; + const gbColor3 = contextValue.getElementStyles( { + data: createMockDataWithGroup( 'great-britain' ), + index: 1, + } ).color; + const jpColor3 = contextValue.getElementStyles( { + data: createMockDataWithGroup( 'japan' ), + index: 2, + } ).color; // All colors should remain stable throughout the process expect( usColor1 ).toBe( usColor2 ); @@ -548,7 +631,7 @@ describe( 'ChartContext', () => { registerChart: GlobalChartsContextValue[ 'registerChart' ]; unregisterChart: GlobalChartsContextValue[ 'unregisterChart' ]; getChartData: GlobalChartsContextValue[ 'getChartData' ]; - resolveColor: GlobalChartsContextValue[ 'resolveColor' ]; + getElementStyles: GlobalChartsContextValue[ 'getElementStyles' ]; } > = []; const TestComponent = () => { @@ -557,7 +640,7 @@ describe( 'ChartContext', () => { registerChart: context.registerChart, unregisterChart: context.unregisterChart, getChartData: context.getChartData, - resolveColor: context.resolveColor, + getElementStyles: context.getElementStyles, } ); return
Test
; }; @@ -578,7 +661,7 @@ describe( 'ChartContext', () => { expect( functionRefs[ 0 ].registerChart ).toBe( functionRefs[ 1 ].registerChart ); expect( functionRefs[ 0 ].unregisterChart ).toBe( functionRefs[ 1 ].unregisterChart ); expect( functionRefs[ 0 ].getChartData ).toBe( functionRefs[ 1 ].getChartData ); - expect( functionRefs[ 0 ].resolveColor ).toBe( functionRefs[ 1 ].resolveColor ); + expect( functionRefs[ 0 ].getElementStyles ).toBe( functionRefs[ 1 ].getElementStyles ); } ); } ); } ); From a57df8c5a16cc1533b09fff8a66215a1f02e3d0d Mon Sep 17 00:00:00 2001 From: Adam Wood <1017872+adamwoodnz@users.noreply.github.com> Date: Thu, 18 Sep 2025 11:18:33 +1200 Subject: [PATCH 11/23] Use getElementStyles in useChartLegendItems --- .../hooks/use-leaderboard-legend-items.ts | 4 - .../use-leaderboard-legend-items.test.tsx | 25 ++-- .../legend/hooks/use-chart-legend-items.ts | 120 +++++++++--------- .../components/legend/private/base-legend.tsx | 41 +----- .../charts/src/components/legend/types.ts | 4 - .../src/components/line-chart/line-chart.tsx | 2 +- .../chart-context/global-charts-provider.tsx | 6 +- .../src/providers/chart-context/index.ts | 7 +- .../src/providers/chart-context/types.ts | 2 +- 9 files changed, 89 insertions(+), 122 deletions(-) diff --git a/projects/js-packages/charts/src/components/leaderboard-chart/hooks/use-leaderboard-legend-items.ts b/projects/js-packages/charts/src/components/leaderboard-chart/hooks/use-leaderboard-legend-items.ts index 832aad283a94f..9a876b3cd6cd8 100644 --- a/projects/js-packages/charts/src/components/leaderboard-chart/hooks/use-leaderboard-legend-items.ts +++ b/projects/js-packages/charts/src/components/leaderboard-chart/hooks/use-leaderboard-legend-items.ts @@ -55,8 +55,6 @@ export function useLeaderboardLegendItems( { label: legendLabels?.primary || __( 'Current period', 'jetpack-charts' ), value: '', color: resolvedPrimaryColor, - index: 0, - overrideColor: primaryColor, } ); // Add comparison period legend item if comparison is enabled and overlay label is not enabled @@ -70,8 +68,6 @@ export function useLeaderboardLegendItems( { label: legendLabels?.comparison || __( 'Previous period', 'jetpack-charts' ), value: '', color: resolvedSecondaryColor, - index: 1, - overrideColor: secondaryColor, } ); } diff --git a/projects/js-packages/charts/src/components/leaderboard-chart/test/use-leaderboard-legend-items.test.tsx b/projects/js-packages/charts/src/components/leaderboard-chart/test/use-leaderboard-legend-items.test.tsx index ea494f768824a..f8bf6471a7ac4 100644 --- a/projects/js-packages/charts/src/components/leaderboard-chart/test/use-leaderboard-legend-items.test.tsx +++ b/projects/js-packages/charts/src/components/leaderboard-chart/test/use-leaderboard-legend-items.test.tsx @@ -82,8 +82,6 @@ describe( 'useLeaderboardLegendItems', () => { label: 'Current period', value: '', color: expect.any( String ), - index: 0, - overrideColor: undefined, } ); } ); @@ -106,8 +104,6 @@ describe( 'useLeaderboardLegendItems', () => { label: 'Current period', value: '', color: expect.any( String ), - index: 0, - overrideColor: undefined, } ); // Previous period item @@ -115,8 +111,6 @@ describe( 'useLeaderboardLegendItems', () => { label: 'Previous period', value: '', color: expect.any( String ), - index: 1, - overrideColor: undefined, } ); } ); } ); @@ -143,8 +137,6 @@ describe( 'useLeaderboardLegendItems', () => { // Note: The actual color will be resolved by the context, but we can check structure expect( result.current[ 0 ].color ).toBeTruthy(); expect( result.current[ 1 ].color ).toBeTruthy(); - expect( result.current[ 0 ].overrideColor ).toBeUndefined(); - expect( result.current[ 1 ].overrideColor ).toBeUndefined(); } ); it( 'should use custom primary color override', () => { @@ -161,7 +153,7 @@ describe( 'useLeaderboardLegendItems', () => { { wrapper } ); - expect( result.current[ 0 ].overrideColor ).toBe( customPrimary ); + expect( result.current[ 0 ].color ).toBe( customPrimary ); } ); it( 'should use custom secondary color override', () => { @@ -180,8 +172,8 @@ describe( 'useLeaderboardLegendItems', () => { { wrapper } ); - expect( result.current[ 0 ].overrideColor ).toBe( customPrimary ); - expect( result.current[ 1 ].overrideColor ).toBe( customSecondary ); + expect( result.current[ 0 ].color ).toBe( customPrimary ); + expect( result.current[ 1 ].color ).toBe( customSecondary ); } ); it( 'should use both custom colors with comparison', () => { @@ -201,8 +193,8 @@ describe( 'useLeaderboardLegendItems', () => { ); expect( result.current ).toHaveLength( 2 ); - expect( result.current[ 0 ].overrideColor ).toBe( customPrimary ); - expect( result.current[ 1 ].overrideColor ).toBe( customSecondary ); + expect( result.current[ 0 ].color ).toBe( customPrimary ); + expect( result.current[ 1 ].color ).toBe( customSecondary ); } ); } ); @@ -576,7 +568,7 @@ describe( 'useLeaderboardLegendItems', () => { } ); describe( 'Index and structure validation', () => { - it( 'should have correct indices for items', () => { + it( 'should have correct number of items with comparison', () => { const wrapper = createWrapper(); const { result } = renderHook( () => @@ -588,8 +580,9 @@ describe( 'useLeaderboardLegendItems', () => { { wrapper } ); - expect( result.current[ 0 ].index ).toBe( 0 ); - expect( result.current[ 1 ].index ).toBe( 1 ); + expect( result.current ).toHaveLength( 2 ); + expect( result.current[ 0 ].label ).toBe( 'Current period' ); + expect( result.current[ 1 ].label ).toBe( 'Previous period' ); } ); it( 'should have empty value strings for all items', () => { diff --git a/projects/js-packages/charts/src/components/legend/hooks/use-chart-legend-items.ts b/projects/js-packages/charts/src/components/legend/hooks/use-chart-legend-items.ts index dc950e1b08a92..2b3452e725fa8 100644 --- a/projects/js-packages/charts/src/components/legend/hooks/use-chart-legend-items.ts +++ b/projects/js-packages/charts/src/components/legend/hooks/use-chart-legend-items.ts @@ -1,11 +1,14 @@ import { useMemo } from 'react'; -import { useGlobalChartsTheme } from '../../../providers'; -import { getItemShapeStyles, getSeriesStroke, formatPercentage } from '../../../utils'; -import type { ChartTheme, SeriesData, DataPointDate, DataPointPercentage } from '../../../types'; +import { + useGlobalChartsContext, + type GetElementStylesParams, + type ElementStyles, +} from '../../../providers'; +import { formatPercentage } from '../../../utils'; +import type { SeriesData, DataPointDate, DataPointPercentage } from '../../../types'; import type { BaseLegendItem } from '../types'; -import type { LegendShape } from '@visx/legend/lib/types'; -import type { GlyphProps } from '@visx/xychart'; -import type { ReactNode } from 'react'; +import type { GlyphProps, LineStyles } from '@visx/xychart'; +import type { ReactNode, CSSProperties } from 'react'; export type LegendValueDisplay = 'percentage' | 'value' | 'valueDisplay' | 'none'; @@ -58,42 +61,44 @@ function formatPointValue( /** * Processes SeriesData into legend items - * @param seriesData - The series data to process - * @param theme - The chart theme for colors - * @param showValues - Whether to show values in legend - * @param withGlyph - Whether to include glyph rendering - * @param glyphSize - Size of the glyph - * @param renderGlyph - Component to render the glyph - * @param legendShape - The shape to use for the legend + * @param seriesData - The series data to process + * @param getElementStyles - Function to get element styles + * @param showValues - Whether to show values in legend + * @param withGlyph - Whether to include glyph rendering + * @param glyphSize - Size of the glyph + * @param renderGlyph - Component to render the glyph * @return Array of processed legend items */ function processSeriesData( seriesData: SeriesData[], - theme: ChartTheme, + getElementStyles: ( params: GetElementStylesParams ) => ElementStyles, showValues: boolean, withGlyph: boolean, glyphSize: number, - renderGlyph?: < Datum extends object >( props: GlyphProps< Datum > ) => ReactNode, - legendShape?: LegendShape< SeriesData[], number > + renderGlyph?: < Datum extends object >( props: GlyphProps< Datum > ) => ReactNode ): BaseLegendItem[] { const mapper = ( series: SeriesData, index: number ) => { - const { shapeStyles } = getItemShapeStyles( series, index, theme, legendShape ); - const baseItem = { + const { color, lineStyles, glyph } = getElementStyles( { + data: series, + index, + } ); + + const baseItem: BaseLegendItem = { label: series.label, value: showValues ? series.data?.length?.toString() || '0' : '', - color: getSeriesStroke( series, index, theme.colors ), - shapeStyle: shapeStyles, - group: series.group, - index, - overrideColor: series.options?.stroke, + color, + shapeStyle: lineStyles as CSSProperties & LineStyles, }; - if ( withGlyph && renderGlyph ) { - return { - ...baseItem, - glyphSize, - renderGlyph, - }; + if ( withGlyph ) { + const glyphToUse = renderGlyph || glyph; + if ( glyphToUse ) { + return { + ...baseItem, + glyphSize, + renderGlyph: glyphToUse, + }; + } } return baseItem; @@ -105,7 +110,7 @@ function processSeriesData( /** * Processes point data into legend items * @param pointData - The point data to process - * @param theme - The chart theme for colors + * @param getElementStyles - Function to get element styles * @param showValues - Whether to show values in legend * @param legendValueDisplay - What type of value to display * @param withGlyph - Whether to include glyph rendering @@ -115,7 +120,7 @@ function processSeriesData( */ function processPointData( pointData: ( DataPointDate | DataPointPercentage )[], - theme: ChartTheme, + getElementStyles: ( params: GetElementStylesParams ) => ElementStyles, showValues: boolean, legendValueDisplay: LegendValueDisplay, withGlyph: boolean, @@ -123,23 +128,27 @@ function processPointData( renderGlyph?: < Datum extends object >( props: GlyphProps< Datum > ) => ReactNode ): BaseLegendItem[] { const mapper = ( point: DataPointDate | DataPointPercentage, index: number ) => { - const baseItem = { + const { color, lineStyles, glyph } = getElementStyles( { + data: point as DataPointPercentage, + index, + } ); + + const baseItem: BaseLegendItem = { label: point.label, value: formatPointValue( point, showValues, legendValueDisplay ), - color: ( point as DataPointPercentage ).color ?? theme.colors[ index % theme.colors.length ], - group: ( point as DataPointPercentage ).group, - index, - overrideColor: ( point as DataPointPercentage ).color, + color, + shapeStyle: lineStyles as CSSProperties & LineStyles, }; - if ( withGlyph && renderGlyph ) { - const itemWithGlyph = { - ...baseItem, - glyphSize, - renderGlyph, - }; - - return itemWithGlyph; + if ( withGlyph ) { + const glyphToUse = renderGlyph || glyph; + if ( glyphToUse ) { + return { + ...baseItem, + glyphSize, + renderGlyph: glyphToUse, + }; + } } return baseItem; @@ -150,18 +159,13 @@ function processPointData( /** * Hook to transform chart data into legend items - * @param data - The chart data to transform - * @param options - Configuration options for legend generation - * @param legendShape - The shape type for legend items (string literal or React component) + * @param data - The chart data to transform + * @param options - Configuration options for legend generation * @return Array of legend items ready for display */ export function useChartLegendItems< T extends SeriesData[] | DataPointDate[] | DataPointPercentage[], ->( - data: T, - options: ChartLegendOptions = {}, - legendShape?: LegendShape< SeriesData[], number > -): BaseLegendItem[] { +>( data: T, options: ChartLegendOptions = {} ): BaseLegendItem[] { const { showValues = false, legendValueDisplay = 'percentage', @@ -169,7 +173,7 @@ export function useChartLegendItems< glyphSize = 8, renderGlyph, } = options; - const theme = useGlobalChartsTheme(); + const { getElementStyles } = useGlobalChartsContext(); return useMemo( () => { if ( ! data || ! Array.isArray( data ) || data.length === 0 ) { @@ -180,19 +184,18 @@ export function useChartLegendItems< if ( 'data' in data[ 0 ] ) { return processSeriesData( data as SeriesData[], - theme, + getElementStyles, showValues, withGlyph, glyphSize, - renderGlyph, - legendShape + renderGlyph ); } // Handle DataPointDate or DataPointPercentage (single data points) return processPointData( data as ( DataPointDate | DataPointPercentage )[], - theme, + getElementStyles, showValues, legendValueDisplay, withGlyph, @@ -201,12 +204,11 @@ export function useChartLegendItems< ); }, [ data, - theme, + getElementStyles, showValues, legendValueDisplay, withGlyph, glyphSize, renderGlyph, - legendShape, ] ); } diff --git a/projects/js-packages/charts/src/components/legend/private/base-legend.tsx b/projects/js-packages/charts/src/components/legend/private/base-legend.tsx index ca9f708d7e018..97d6055585676 100644 --- a/projects/js-packages/charts/src/components/legend/private/base-legend.tsx +++ b/projects/js-packages/charts/src/components/legend/private/base-legend.tsx @@ -2,16 +2,9 @@ import { Group } from '@visx/group'; import { LegendItem, LegendLabel, LegendOrdinal, LegendShape } from '@visx/legend'; import { scaleOrdinal } from '@visx/scale'; import clsx from 'clsx'; -import { - type RefAttributes, - type ForwardRefExoticComponent, - forwardRef, - useCallback, - useMemo, - useContext, -} from 'react'; +import { type RefAttributes, type ForwardRefExoticComponent, forwardRef, useCallback } from 'react'; import { useTextTruncation } from '../../../hooks'; -import { useGlobalChartsTheme, GlobalChartsContext } from '../../../providers'; +import { useGlobalChartsTheme } from '../../../providers'; import { valueOrIdentity, valueOrIdentityString, labelTransformFactory } from '../utils'; import styles from './base-legend.module.scss'; import type { BaseLegendProps } from '../types'; @@ -90,38 +83,16 @@ export const BaseLegend: ForwardRefExoticComponent< ref ) => { const theme = useGlobalChartsTheme(); - const context = useContext( GlobalChartsContext ); - const getElementStyles = context?.getElementStyles; - - // Resolve colors dynamically for items that have group info - const itemsWithResolvedColors = useMemo( () => { - return items.map( item => { - // If item has group info and we have a context, resolve color dynamically - if ( item.group !== undefined && item.index !== undefined && getElementStyles ) { - const { color } = getElementStyles( { - data: item, - index: item.index, - overrideColor: item.overrideColor, - } ); - - return { ...item, color }; - } - // Otherwise use the static color - return item; - } ); - }, [ items, getElementStyles ] ); const legendScale = scaleOrdinal( { - domain: itemsWithResolvedColors.map( item => item.label ), - range: itemsWithResolvedColors.map( item => item.color ), + domain: items.map( item => item.label ), + range: items.map( item => item.color ), } ); const domain = legendScale.domain(); - // For right-aligned vertical legends, use row-reverse to align text consistently - const getShapeStyle = useCallback( - ( { index }: { index: number } ) => itemsWithResolvedColors[ index ]?.shapeStyle, - [ itemsWithResolvedColors ] + ( { index }: { index: number } ) => items[ index ]?.shapeStyle, + [ items ] ); return ( diff --git a/projects/js-packages/charts/src/components/legend/types.ts b/projects/js-packages/charts/src/components/legend/types.ts index 603ccef4f8c05..651a55a0d0984 100644 --- a/projects/js-packages/charts/src/components/legend/types.ts +++ b/projects/js-packages/charts/src/components/legend/types.ts @@ -38,8 +38,4 @@ export type BaseLegendItem = { glyphSize?: number; renderGlyph?: < Datum extends object >( props: GlyphProps< Datum > ) => ReactNode; shapeStyle?: CSSProperties & LineStyles; - // Optional group info for dynamic color resolution - group?: string; - index?: number; - overrideColor?: string; }; diff --git a/projects/js-packages/charts/src/components/line-chart/line-chart.tsx b/projects/js-packages/charts/src/components/line-chart/line-chart.tsx index 507043ad5d4cd..2676528c29bab 100644 --- a/projects/js-packages/charts/src/components/line-chart/line-chart.tsx +++ b/projects/js-packages/charts/src/components/line-chart/line-chart.tsx @@ -332,7 +332,7 @@ const LineChartInternal = forwardRef< SingleChartRef, LineChartProps >( ); // Create legend items using the reusable hook - const legendItems = useChartLegendItems( dataSorted, legendOptions, legendShape ); + const legendItems = useChartLegendItems( dataSorted, legendOptions ); // Memoize metadata to prevent unnecessary re-registration const chartMetadata = useMemo( diff --git a/projects/js-packages/charts/src/providers/chart-context/global-charts-provider.tsx b/projects/js-packages/charts/src/providers/chart-context/global-charts-provider.tsx index 7ad451a6e2406..939dd1c0c2347 100644 --- a/projects/js-packages/charts/src/providers/chart-context/global-charts-provider.tsx +++ b/projects/js-packages/charts/src/providers/chart-context/global-charts-provider.tsx @@ -88,12 +88,16 @@ export const GlobalChartsProvider: FC< GlobalChartsProviderProps > = ( { childre const getElementStyles = useCallback< GlobalChartsContextValue[ 'getElementStyles' ] >( ( { data, index, overrideColor } ) => { const isSeriesData = data && typeof data === 'object' && 'data' in data && 'options' in data; + const isPointPercentageData = data && typeof data === 'object' && 'percentage' in data; return { color: resolveColor( { group: data?.group, index, - overrideColor: overrideColor || ( isSeriesData && data?.options?.stroke ), + overrideColor: + overrideColor || + ( isSeriesData && data?.options?.stroke ) || + ( isPointPercentageData && data?.color ), } ), lineStyles: isSeriesData ? getSeriesLineStyles( data, index, providerTheme ) : {}, glyph: providerTheme.glyphs?.[ index ], diff --git a/projects/js-packages/charts/src/providers/chart-context/index.ts b/projects/js-packages/charts/src/providers/chart-context/index.ts index bd89ef2f46737..83018ad412e73 100644 --- a/projects/js-packages/charts/src/providers/chart-context/index.ts +++ b/projects/js-packages/charts/src/providers/chart-context/index.ts @@ -3,5 +3,10 @@ export { useGlobalChartsContext } from './hooks/use-global-charts-context'; export { useChartId } from './hooks/use-chart-id'; export { useChartRegistration } from './hooks/use-chart-registration'; export { useGlobalChartsTheme } from './hooks/use-global-charts-theme'; -export type { GlobalChartsContextValue, ChartRegistration } from './types'; +export type { + GlobalChartsContextValue, + ChartRegistration, + GetElementStylesParams, + ElementStyles, +} from './types'; export * from './themes'; diff --git a/projects/js-packages/charts/src/providers/chart-context/types.ts b/projects/js-packages/charts/src/providers/chart-context/types.ts index dc5a11e6f4b1a..2bea8835cb61f 100644 --- a/projects/js-packages/charts/src/providers/chart-context/types.ts +++ b/projects/js-packages/charts/src/providers/chart-context/types.ts @@ -11,7 +11,7 @@ export interface ChartRegistration { export type GetElementStylesParams = { index: number; - data?: SeriesData | DataPointPercentage | BaseLegendItem; + data?: SeriesData | DataPointPercentage; overrideColor?: string; }; From 9180049751b7feb233d09b7872c8f112d6945996 Mon Sep 17 00:00:00 2001 From: Adam Wood <1017872+adamwoodnz@users.noreply.github.com> Date: Thu, 18 Sep 2025 11:28:46 +1200 Subject: [PATCH 12/23] Update legend stories to use different shapes --- .../src/components/legend/stories/index.stories.tsx | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/projects/js-packages/charts/src/components/legend/stories/index.stories.tsx b/projects/js-packages/charts/src/components/legend/stories/index.stories.tsx index d69864d2b050d..0d767d5e2e1c3 100644 --- a/projects/js-packages/charts/src/components/legend/stories/index.stories.tsx +++ b/projects/js-packages/charts/src/components/legend/stories/index.stories.tsx @@ -177,7 +177,7 @@ const WithLineChartData = () => { withGradientFill={ false } withLegendGlyph={ false } /> - + ); }; @@ -231,7 +231,7 @@ const StandaloneLegendWithChartIdComponent = () => { withLegendGlyph={ false } /> { /* Standalone legend that automatically gets data from chart context */ } - + ); }; @@ -365,7 +365,7 @@ const DashboardWithCentralizedLegend = () => { > Revenue Trends - +
@@ -391,7 +391,7 @@ const DashboardWithCentralizedLegend = () => { > Device Distribution - +
@@ -551,7 +551,7 @@ This interactive story demonstrates all the text overflow and wrapping features - **Text Overflow Modes**: - **Wrap** (default): Text wraps naturally to multiple lines when it exceeds maxWidth - **Ellipsis**: Truncates text with ellipsis (...) and shows tooltip on hover - + - **Orientation**: Switch between horizontal and vertical layouts - **Max Width**: Adjust the maximum width constraint with the slider (50-300px) - **Position & Alignment**: Control legend placement From 204be2a80758081170467bc9935a930e645b923b Mon Sep 17 00:00:00 2001 From: Adam Wood <1017872+adamwoodnz@users.noreply.github.com> Date: Thu, 18 Sep 2025 12:16:57 +1200 Subject: [PATCH 13/23] Revert removal of legend shape styles handling --- .../legend/hooks/use-chart-legend-items.ts | 42 +++++++++++++------ .../src/components/line-chart/line-chart.tsx | 2 +- .../chart-context/global-charts-provider.tsx | 7 +++- .../src/providers/chart-context/types.ts | 5 ++- 4 files changed, 39 insertions(+), 17 deletions(-) diff --git a/projects/js-packages/charts/src/components/legend/hooks/use-chart-legend-items.ts b/projects/js-packages/charts/src/components/legend/hooks/use-chart-legend-items.ts index 2b3452e725fa8..6be692f55f117 100644 --- a/projects/js-packages/charts/src/components/legend/hooks/use-chart-legend-items.ts +++ b/projects/js-packages/charts/src/components/legend/hooks/use-chart-legend-items.ts @@ -7,8 +7,9 @@ import { import { formatPercentage } from '../../../utils'; import type { SeriesData, DataPointDate, DataPointPercentage } from '../../../types'; import type { BaseLegendItem } from '../types'; -import type { GlyphProps, LineStyles } from '@visx/xychart'; -import type { ReactNode, CSSProperties } from 'react'; +import type { LegendShape } from '@visx/legend/lib/types'; +import type { GlyphProps } from '@visx/xychart'; +import type { ReactNode } from 'react'; export type LegendValueDisplay = 'percentage' | 'value' | 'valueDisplay' | 'none'; @@ -18,6 +19,7 @@ export interface ChartLegendOptions { renderGlyph?: < Datum extends object >( props: GlyphProps< Datum > ) => ReactNode; showValues?: boolean; legendValueDisplay?: LegendValueDisplay; + legendShape?: LegendShape< SeriesData[], number >; } /** @@ -67,6 +69,7 @@ function formatPointValue( * @param withGlyph - Whether to include glyph rendering * @param glyphSize - Size of the glyph * @param renderGlyph - Component to render the glyph + * @param legendShape - The shape type for legend items (string literal or React component) * @return Array of processed legend items */ function processSeriesData( @@ -75,19 +78,21 @@ function processSeriesData( showValues: boolean, withGlyph: boolean, glyphSize: number, - renderGlyph?: < Datum extends object >( props: GlyphProps< Datum > ) => ReactNode + renderGlyph?: < Datum extends object >( props: GlyphProps< Datum > ) => ReactNode, + legendShape?: LegendShape< SeriesData[], number > ): BaseLegendItem[] { const mapper = ( series: SeriesData, index: number ) => { - const { color, lineStyles, glyph } = getElementStyles( { + const { color, glyph, shapeStyles } = getElementStyles( { data: series, index, + legendShape, } ); const baseItem: BaseLegendItem = { label: series.label, value: showValues ? series.data?.length?.toString() || '0' : '', color, - shapeStyle: lineStyles as CSSProperties & LineStyles, + shapeStyle: shapeStyles, }; if ( withGlyph ) { @@ -116,6 +121,7 @@ function processSeriesData( * @param withGlyph - Whether to include glyph rendering * @param glyphSize - Size of the glyph * @param renderGlyph - Component to render the glyph + * @param legendShape - The shape type for legend items (string literal or React component) * @return Array of processed legend items */ function processPointData( @@ -125,19 +131,21 @@ function processPointData( legendValueDisplay: LegendValueDisplay, withGlyph: boolean, glyphSize: number, - renderGlyph?: < Datum extends object >( props: GlyphProps< Datum > ) => ReactNode + renderGlyph?: < Datum extends object >( props: GlyphProps< Datum > ) => ReactNode, + legendShape?: LegendShape< SeriesData[], number > ): BaseLegendItem[] { const mapper = ( point: DataPointDate | DataPointPercentage, index: number ) => { - const { color, lineStyles, glyph } = getElementStyles( { + const { color, glyph, shapeStyles } = getElementStyles( { data: point as DataPointPercentage, index, + legendShape, } ); const baseItem: BaseLegendItem = { label: point.label, value: formatPointValue( point, showValues, legendValueDisplay ), color, - shapeStyle: lineStyles as CSSProperties & LineStyles, + shapeStyle: shapeStyles, }; if ( withGlyph ) { @@ -159,13 +167,18 @@ function processPointData( /** * Hook to transform chart data into legend items - * @param data - The chart data to transform - * @param options - Configuration options for legend generation + * @param data - The chart data to transform + * @param options - Configuration options for legend generation + * @param legendShape - The shape type for legend items (string literal or React component) * @return Array of legend items ready for display */ export function useChartLegendItems< T extends SeriesData[] | DataPointDate[] | DataPointPercentage[], ->( data: T, options: ChartLegendOptions = {} ): BaseLegendItem[] { +>( + data: T, + options: ChartLegendOptions = {}, + legendShape?: LegendShape< SeriesData[], number > +): BaseLegendItem[] { const { showValues = false, legendValueDisplay = 'percentage', @@ -188,7 +201,8 @@ export function useChartLegendItems< showValues, withGlyph, glyphSize, - renderGlyph + renderGlyph, + legendShape ); } @@ -200,7 +214,8 @@ export function useChartLegendItems< legendValueDisplay, withGlyph, glyphSize, - renderGlyph + renderGlyph, + legendShape ); }, [ data, @@ -210,5 +225,6 @@ export function useChartLegendItems< withGlyph, glyphSize, renderGlyph, + legendShape, ] ); } diff --git a/projects/js-packages/charts/src/components/line-chart/line-chart.tsx b/projects/js-packages/charts/src/components/line-chart/line-chart.tsx index 2676528c29bab..507043ad5d4cd 100644 --- a/projects/js-packages/charts/src/components/line-chart/line-chart.tsx +++ b/projects/js-packages/charts/src/components/line-chart/line-chart.tsx @@ -332,7 +332,7 @@ const LineChartInternal = forwardRef< SingleChartRef, LineChartProps >( ); // Create legend items using the reusable hook - const legendItems = useChartLegendItems( dataSorted, legendOptions ); + const legendItems = useChartLegendItems( dataSorted, legendOptions, legendShape ); // Memoize metadata to prevent unnecessary re-registration const chartMetadata = useMemo( diff --git a/projects/js-packages/charts/src/providers/chart-context/global-charts-provider.tsx b/projects/js-packages/charts/src/providers/chart-context/global-charts-provider.tsx index 939dd1c0c2347..0256f5d07fedd 100644 --- a/projects/js-packages/charts/src/providers/chart-context/global-charts-provider.tsx +++ b/projects/js-packages/charts/src/providers/chart-context/global-charts-provider.tsx @@ -1,5 +1,5 @@ import { createContext, useCallback, useMemo, useState, useEffect, useRef } from 'react'; -import { getSeriesLineStyles, mergeThemes } from '../../utils'; +import { getItemShapeStyles, getSeriesLineStyles, mergeThemes } from '../../utils'; import { defaultTheme } from './themes'; import type { GlobalChartsContextValue, ChartRegistration } from './types'; import type { ChartTheme, CompleteChartTheme } from '../../types'; @@ -86,7 +86,7 @@ export const GlobalChartsProvider: FC< GlobalChartsProviderProps > = ( { childre ); const getElementStyles = useCallback< GlobalChartsContextValue[ 'getElementStyles' ] >( - ( { data, index, overrideColor } ) => { + ( { data, index, overrideColor, legendShape } ) => { const isSeriesData = data && typeof data === 'object' && 'data' in data && 'options' in data; const isPointPercentageData = data && typeof data === 'object' && 'percentage' in data; @@ -101,6 +101,9 @@ export const GlobalChartsProvider: FC< GlobalChartsProviderProps > = ( { childre } ), lineStyles: isSeriesData ? getSeriesLineStyles( data, index, providerTheme ) : {}, glyph: providerTheme.glyphs?.[ index ], + shapeStyles: isSeriesData + ? getItemShapeStyles( data, index, providerTheme, legendShape ) + : {}, }; }, [ providerTheme, resolveColor ] diff --git a/projects/js-packages/charts/src/providers/chart-context/types.ts b/projects/js-packages/charts/src/providers/chart-context/types.ts index 2bea8835cb61f..2ee3377ee437b 100644 --- a/projects/js-packages/charts/src/providers/chart-context/types.ts +++ b/projects/js-packages/charts/src/providers/chart-context/types.ts @@ -1,6 +1,7 @@ -import { ReactNode } from 'react'; +import { CSSProperties, ReactNode } from 'react'; import type { BaseLegendItem } from '../../components/legend'; import type { CompleteChartTheme, DataPointPercentage, SeriesData } from '../../types'; +import type { LegendShape } from '@visx/legend/lib/types'; import type { GlyphProps, LineStyles } from '@visx/xychart'; export interface ChartRegistration { @@ -13,12 +14,14 @@ export type GetElementStylesParams = { index: number; data?: SeriesData | DataPointPercentage; overrideColor?: string; + legendShape?: LegendShape< SeriesData[], number >; }; export type ElementStyles = { color: string; lineStyles: LineStyles; glyph: < Datum extends object >( props: GlyphProps< Datum > ) => ReactNode; + shapeStyles: CSSProperties & LineStyles; }; export interface GlobalChartsContextValue { From eace67ac6f33eb7f8d262dbb97500aa521770b1d Mon Sep 17 00:00:00 2001 From: Adam Wood <1017872+adamwoodnz@users.noreply.github.com> Date: Thu, 18 Sep 2025 12:37:54 +1200 Subject: [PATCH 14/23] Remove override color from pie charts --- .../js-packages/charts/src/components/pie-chart/pie-chart.tsx | 4 +--- .../pie-semi-circle-chart/pie-semi-circle-chart.tsx | 2 +- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/projects/js-packages/charts/src/components/pie-chart/pie-chart.tsx b/projects/js-packages/charts/src/components/pie-chart/pie-chart.tsx index 0161c6e0ba99a..645454ff3b765 100644 --- a/projects/js-packages/charts/src/components/pie-chart/pie-chart.tsx +++ b/projects/js-packages/charts/src/components/pie-chart/pie-chart.tsx @@ -215,10 +215,8 @@ const PieChartInternal = ( { const accessors = { value: ( d: DataPointPercentage ) => d.value, - // Use the color property from the data object as a last resort. The theme provides colours by default. fill: ( d: DataPointPercentage & { index: number } ) => { - const { index, color: overrideColor } = d; - return getElementStyles( { data: d, index, overrideColor } ).color; + return getElementStyles( { data: d, index: d.index } ).color; }, }; diff --git a/projects/js-packages/charts/src/components/pie-semi-circle-chart/pie-semi-circle-chart.tsx b/projects/js-packages/charts/src/components/pie-semi-circle-chart/pie-semi-circle-chart.tsx index 8e8324119ce5c..423e20b738660 100644 --- a/projects/js-packages/charts/src/components/pie-semi-circle-chart/pie-semi-circle-chart.tsx +++ b/projects/js-packages/charts/src/components/pie-semi-circle-chart/pie-semi-circle-chart.tsx @@ -171,7 +171,7 @@ const PieSemiCircleChartInternal: FC< PieSemiCircleChartProps > = ( { b: DataPointPercentage & { index: number } ) => b.value - a.value, fill: ( d: DataPointPercentage & { index: number } ) => - getElementStyles( { data: d, index: d.index, overrideColor: d.color } ).color, + getElementStyles( { data: d, index: d.index } ).color, } ), [ getElementStyles ] ); From dae27934dd16d5912bcf84aa23bdbc405471ac6b Mon Sep 17 00:00:00 2001 From: Adam Wood <1017872+adamwoodnz@users.noreply.github.com> Date: Thu, 18 Sep 2025 12:57:48 +1200 Subject: [PATCH 15/23] Simplify getItemShapeStyles return type --- .../charts/src/utils/get-styles.ts | 6 ++-- .../charts/src/utils/test/get-styles.test.ts | 28 +++++++++---------- 2 files changed, 17 insertions(+), 17 deletions(-) diff --git a/projects/js-packages/charts/src/utils/get-styles.ts b/projects/js-packages/charts/src/utils/get-styles.ts index 57e3fc5ba5cd9..f0148af06ec12 100644 --- a/projects/js-packages/charts/src/utils/get-styles.ts +++ b/projects/js-packages/charts/src/utils/get-styles.ts @@ -60,7 +60,7 @@ export function getItemShapeStyles( index: number, theme: ChartTheme, legendShape?: LegendShape< SeriesData[], number > -): { shapeStyles: CSSProperties & LineStyles } { +): CSSProperties & LineStyles { const seriesShapeStyles = series.options?.legendShapeStyle ?? {}; const lineStyles = legendShape === 'line' ? getSeriesLineStyles( series, index, theme ) : {}; const themeShapeStyles = theme.legendShapeStyles?.[ index ]; @@ -76,9 +76,9 @@ export function getItemShapeStyles( value => value !== undefined && value !== null && value !== '' ) ) { - return { shapeStyles: itemShapeStyles as CSSProperties & LineStyles }; + return itemShapeStyles as CSSProperties & LineStyles; } // Fallback to theme shape styles if defined - return { shapeStyles: themeShapeStyles ?? {} }; + return themeShapeStyles ?? {}; } diff --git a/projects/js-packages/charts/src/utils/test/get-styles.test.ts b/projects/js-packages/charts/src/utils/test/get-styles.test.ts index d1f62d89455b3..dd4b1669ee71f 100644 --- a/projects/js-packages/charts/src/utils/test/get-styles.test.ts +++ b/projects/js-packages/charts/src/utils/test/get-styles.test.ts @@ -131,7 +131,7 @@ describe( 'Series styling utility functions', () => { }; const result = getItemShapeStyles( seriesWithShapeStyle, 0, mockTheme as ChartTheme, 'rect' ); - expect( result.shapeStyles ).toEqual( customShapeStyle ); + expect( result ).toEqual( customShapeStyle ); } ); it( 'combines line styles when legendShape is "line"', () => { @@ -141,7 +141,7 @@ describe( 'Series styling utility functions', () => { }; const result = getItemShapeStyles( comparisonSeries, 0, mockTheme as ChartTheme, 'line' ); - expect( result.shapeStyles ).toEqual( { + expect( result ).toEqual( { strokeDasharray: '4 4', strokeLinecap: 'square', strokeWidth: 1.5, @@ -155,7 +155,7 @@ describe( 'Series styling utility functions', () => { }; const result = getItemShapeStyles( comparisonSeries, 0, mockTheme as ChartTheme, 'rect' ); - expect( result.shapeStyles ).toEqual( mockTheme.legendShapeStyles[ 0 ] ); + expect( result ).toEqual( mockTheme.legendShapeStyles[ 0 ] ); } ); it( 'merges custom shape styles with line styles for line shape', () => { @@ -169,7 +169,7 @@ describe( 'Series styling utility functions', () => { }; const result = getItemShapeStyles( comparisonSeries, 0, mockTheme as ChartTheme, 'line' ); - expect( result.shapeStyles ).toEqual( { + expect( result ).toEqual( { fill: '#CUSTOM', strokeDasharray: '4 4', strokeLinecap: 'square', @@ -189,7 +189,7 @@ describe( 'Series styling utility functions', () => { mockTheme as ChartTheme, 'rect' ); - expect( result.shapeStyles ).toEqual( mockTheme.legendShapeStyles[ 1 ] ); + expect( result ).toEqual( mockTheme.legendShapeStyles[ 1 ] ); } ); it( 'returns empty object when no theme shape styles and no meaningful custom styles', () => { @@ -199,7 +199,7 @@ describe( 'Series styling utility functions', () => { } as ChartTheme; const result = getItemShapeStyles( mockSeriesData, 0, themeWithoutShapeStyles, 'rect' ); - expect( result.shapeStyles ).toEqual( {} ); + expect( result ).toEqual( {} ); } ); it( 'returns custom styles even with undefined/null values when meaningful values exist', () => { @@ -221,7 +221,7 @@ describe( 'Series styling utility functions', () => { mockTheme as ChartTheme, 'rect' ); - expect( result.shapeStyles ).toEqual( { + expect( result ).toEqual( { fill: '#VALID', stroke: '', strokeWidth: undefined, @@ -242,7 +242,7 @@ describe( 'Series styling utility functions', () => { }; const result = getItemShapeStyles( complexSeries, 0, mockTheme as ChartTheme, 'line' ); - expect( result.shapeStyles ).toEqual( { + expect( result ).toEqual( { fill: '#OVERRIDE', strokeWidth: 1.5, // Semantic style wins over custom style strokeDasharray: '4 4', @@ -252,12 +252,12 @@ describe( 'Series styling utility functions', () => { it( 'returns empty object when index exceeds theme shape styles array length', () => { const result = getItemShapeStyles( mockSeriesData, 3, mockTheme as ChartTheme, 'rect' ); - expect( result.shapeStyles ).toEqual( {} ); // index 3 doesn't exist in array of length 2 + expect( result ).toEqual( {} ); // index 3 doesn't exist in array of length 2 } ); it( 'works without legendShape parameter', () => { const result = getItemShapeStyles( mockSeriesData, 0, mockTheme as ChartTheme ); - expect( result.shapeStyles ).toEqual( mockTheme.legendShapeStyles[ 0 ] ); + expect( result ).toEqual( mockTheme.legendShapeStyles[ 0 ] ); } ); it( 'handles other legendShape string values like "circle"', () => { @@ -268,7 +268,7 @@ describe( 'Series styling utility functions', () => { const result = getItemShapeStyles( comparisonSeries, 0, mockTheme as ChartTheme, 'circle' ); // Should not include line styles for circle shape - expect( result.shapeStyles ).toEqual( mockTheme.legendShapeStyles[ 0 ] ); + expect( result ).toEqual( mockTheme.legendShapeStyles[ 0 ] ); } ); it( 'handles React component as legendShape parameter', () => { @@ -285,7 +285,7 @@ describe( 'Series styling utility functions', () => { CustomShape ); // Should not include line styles for component shape - expect( result.shapeStyles ).toEqual( mockTheme.legendShapeStyles[ 0 ] ); + expect( result ).toEqual( mockTheme.legendShapeStyles[ 0 ] ); } ); it( 'prioritizes series line styles over theme line styles when legendShape is line', () => { @@ -302,7 +302,7 @@ describe( 'Series styling utility functions', () => { mockTheme as ChartTheme, 'line' ); - expect( result.shapeStyles ).toEqual( { + expect( result ).toEqual( { strokeWidth: 99, strokeDasharray: '10 10', } ); @@ -324,7 +324,7 @@ describe( 'Series styling utility functions', () => { 'line' ); // Should return line styles even though legendShapeStyle is empty - expect( result.shapeStyles ).toEqual( { + expect( result ).toEqual( { strokeDasharray: '4 4', strokeLinecap: 'square', strokeWidth: 1.5, From 92ed0da929344d6b0ca46c1bece30449e439a6c8 Mon Sep 17 00:00:00 2001 From: Adam Wood <1017872+adamwoodnz@users.noreply.github.com> Date: Thu, 18 Sep 2025 13:08:58 +1200 Subject: [PATCH 16/23] Loosen type of legendShapeStyles --- projects/js-packages/charts/src/types.ts | 2 +- projects/js-packages/charts/src/utils/get-styles.ts | 7 +++---- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/projects/js-packages/charts/src/types.ts b/projects/js-packages/charts/src/types.ts index 6c4c455d0c2c9..98a0fa5d2952f 100644 --- a/projects/js-packages/charts/src/types.ts +++ b/projects/js-packages/charts/src/types.ts @@ -163,7 +163,7 @@ export type ChartTheme = { /** Styles for series lines */ seriesLineStyles?: LineStyles[]; /** Styles for legend shapes */ - legendShapeStyles?: ( CSSProperties & LineStyles )[]; + legendShapeStyles?: Record< string, unknown >[]; /** Array of render functions for glyphs */ glyphs?: Array< < Datum extends object >( props: GlyphProps< Datum > ) => ReactNode >; /** Styles for legend labels */ diff --git a/projects/js-packages/charts/src/utils/get-styles.ts b/projects/js-packages/charts/src/utils/get-styles.ts index f0148af06ec12..1c0dc4191df61 100644 --- a/projects/js-packages/charts/src/utils/get-styles.ts +++ b/projects/js-packages/charts/src/utils/get-styles.ts @@ -1,7 +1,6 @@ import type { ChartTheme, SeriesData } from '../types'; import type { LegendShape } from '@visx/legend/lib/types'; import type { LineStyles } from '@visx/xychart'; -import type { CSSProperties } from 'react'; /** * Utility function to get consolidated line styles for a series @@ -53,14 +52,14 @@ export function getSeriesStroke( * @param {number} index - The index of the series in the data array * @param {ChartTheme} theme - The chart theme configuration * @param {LegendShape} legendShape - The shape to use for the item (optional) - * @return {CSSProperties} The shape styles for the item + * @return {Record< string, unknown >} The shape styles for the item */ export function getItemShapeStyles( series: SeriesData, index: number, theme: ChartTheme, legendShape?: LegendShape< SeriesData[], number > -): CSSProperties & LineStyles { +): Record< string, unknown > { const seriesShapeStyles = series.options?.legendShapeStyle ?? {}; const lineStyles = legendShape === 'line' ? getSeriesLineStyles( series, index, theme ) : {}; const themeShapeStyles = theme.legendShapeStyles?.[ index ]; @@ -76,7 +75,7 @@ export function getItemShapeStyles( value => value !== undefined && value !== null && value !== '' ) ) { - return itemShapeStyles as CSSProperties & LineStyles; + return itemShapeStyles; } // Fallback to theme shape styles if defined From 5db07d01e54f06678cba5705aefbead4ddf875f7 Mon Sep 17 00:00:00 2001 From: Adam Wood <1017872+adamwoodnz@users.noreply.github.com> Date: Thu, 18 Sep 2025 16:29:55 +1200 Subject: [PATCH 17/23] Fix legend color update lag --- .../chart-context/global-charts-provider.tsx | 28 +++++++++---------- .../chart-context/test/chart-context.test.tsx | 18 ++++++------ 2 files changed, 24 insertions(+), 22 deletions(-) diff --git a/projects/js-packages/charts/src/providers/chart-context/global-charts-provider.tsx b/projects/js-packages/charts/src/providers/chart-context/global-charts-provider.tsx index 0256f5d07fedd..7bfe90def26a8 100644 --- a/projects/js-packages/charts/src/providers/chart-context/global-charts-provider.tsx +++ b/projects/js-packages/charts/src/providers/chart-context/global-charts-provider.tsx @@ -1,4 +1,4 @@ -import { createContext, useCallback, useMemo, useState, useEffect, useRef } from 'react'; +import { createContext, useCallback, useMemo, useState, useEffect } from 'react'; import { getItemShapeStyles, getSeriesLineStyles, mergeThemes } from '../../utils'; import { defaultTheme } from './themes'; import type { GlobalChartsContextValue, ChartRegistration } from './types'; @@ -9,24 +9,24 @@ export const GlobalChartsContext = createContext< GlobalChartsContextValue | nul export interface GlobalChartsProviderProps { children: ReactNode; - /** Optional theme override. Considered static for provider lifecycle. */ theme?: Partial< ChartTheme >; } export const GlobalChartsProvider: FC< GlobalChartsProviderProps > = ( { children, theme } ) => { const [ charts, setCharts ] = useState< Map< string, ChartRegistration > >( () => new Map() ); - const providerTheme: CompleteChartTheme = useMemo( - () => ( theme ? mergeThemes( defaultTheme, theme ) : defaultTheme ), - [ theme ] - ); + const providerTheme: CompleteChartTheme = useMemo( () => { + return theme ? mergeThemes( defaultTheme, theme ) : defaultTheme; + }, [ theme ] ); - // Stable group -> color mapping for this provider lifecycle - const groupToColorMapRef = useRef< Map< string, string > >( new Map() ); + const [ groupToColorMap, setGroupToColorMap ] = useState< Map< string, string > >( + () => new Map() + ); - // Reset group color mappings when theme changes + // Reset group color mappings when theme colors change useEffect( () => { - groupToColorMapRef.current = new Map(); + // Create a completely new Map instance to trigger dependencies, e.g. useChartLegendItems + setGroupToColorMap( new Map() ); }, [ providerTheme.colors ] ); const registerChart = useCallback( ( id: string, data: ChartRegistration ) => { @@ -67,22 +67,22 @@ export const GlobalChartsProvider: FC< GlobalChartsProviderProps > = ( { childre // If group provided, maintain a stable assignment if ( group ) { - const existing = groupToColorMapRef.current.get( group ); + const existing = groupToColorMap.get( group ); if ( existing ) { return existing; } // Assign next color from palette in a deterministic cycling manner - const assignedCount = groupToColorMapRef.current.size; + const assignedCount = groupToColorMap.size; const color = colors.length > 0 ? colors[ assignedCount % colors.length ] : '#000000'; - groupToColorMapRef.current.set( group, color ); + groupToColorMap.set( group, color ); return color; } // Fallback: index-based color cycling return colors.length > 0 ? colors[ ( index || 0 ) % colors.length ] : '#000000'; }, - [ providerTheme ] + [ providerTheme, groupToColorMap ] ); const getElementStyles = useCallback< GlobalChartsContextValue[ 'getElementStyles' ] >( diff --git a/projects/js-packages/charts/src/providers/chart-context/test/chart-context.test.tsx b/projects/js-packages/charts/src/providers/chart-context/test/chart-context.test.tsx index 15a5e4e75f660..40c7790ff57d8 100644 --- a/projects/js-packages/charts/src/providers/chart-context/test/chart-context.test.tsx +++ b/projects/js-packages/charts/src/providers/chart-context/test/chart-context.test.tsx @@ -626,7 +626,7 @@ describe( 'ChartContext', () => { } ); describe( 'Context stability', () => { - it( 'maintains stable function references', () => { + it( 'maintains stable function references when no theme changes', () => { const functionRefs: Array< { registerChart: GlobalChartsContextValue[ 'registerChart' ]; unregisterChart: GlobalChartsContextValue[ 'unregisterChart' ]; @@ -646,22 +646,24 @@ describe( 'ChartContext', () => { }; const { rerender } = render( - + ); rerender( - + ); - expect( functionRefs ).toHaveLength( 2 ); - expect( functionRefs[ 0 ].registerChart ).toBe( functionRefs[ 1 ].registerChart ); - expect( functionRefs[ 0 ].unregisterChart ).toBe( functionRefs[ 1 ].unregisterChart ); - expect( functionRefs[ 0 ].getChartData ).toBe( functionRefs[ 1 ].getChartData ); - expect( functionRefs[ 0 ].getElementStyles ).toBe( functionRefs[ 1 ].getElementStyles ); + // After initial mount and theme effect, function refs should be stable across re-renders + const lastTwoRefs = functionRefs.slice( -2 ); + expect( lastTwoRefs ).toHaveLength( 2 ); + expect( lastTwoRefs[ 0 ].registerChart ).toBe( lastTwoRefs[ 1 ].registerChart ); + expect( lastTwoRefs[ 0 ].unregisterChart ).toBe( lastTwoRefs[ 1 ].unregisterChart ); + expect( lastTwoRefs[ 0 ].getChartData ).toBe( lastTwoRefs[ 1 ].getChartData ); + expect( lastTwoRefs[ 0 ].getElementStyles ).toBe( lastTwoRefs[ 1 ].getElementStyles ); } ); } ); } ); From bf97bbe69ce6bdd00b3215df8908f67a9dcfd2eb Mon Sep 17 00:00:00 2001 From: Adam Wood <1017872+adamwoodnz@users.noreply.github.com> Date: Thu, 18 Sep 2025 17:10:44 +1200 Subject: [PATCH 18/23] Add tests for getElementStyles --- .../chart-context/test/chart-context.test.tsx | 590 ++++++++++++++++++ 1 file changed, 590 insertions(+) diff --git a/projects/js-packages/charts/src/providers/chart-context/test/chart-context.test.tsx b/projects/js-packages/charts/src/providers/chart-context/test/chart-context.test.tsx index 40c7790ff57d8..67ee0272aba44 100644 --- a/projects/js-packages/charts/src/providers/chart-context/test/chart-context.test.tsx +++ b/projects/js-packages/charts/src/providers/chart-context/test/chart-context.test.tsx @@ -666,4 +666,594 @@ describe( 'ChartContext', () => { expect( lastTwoRefs[ 0 ].getElementStyles ).toBe( lastTwoRefs[ 1 ].getElementStyles ); } ); } ); + + describe( 'getElementStyles - DataPointPercentage handling', () => { + it( 'handles DataPointPercentage data with color override', () => { + let contextValue: GlobalChartsContextValue; + + const TestComponent = () => { + contextValue = useGlobalChartsContext(); + return
Test
; + }; + + render( + + + + ); + + const percentageDataWithColor = { + label: 'Custom Color Point', + value: 100, + percentage: 50, + color: '#ff9900', + }; + + const styles = contextValue.getElementStyles( { + data: percentageDataWithColor, + index: 0, + } ); + + expect( styles.color ).toBe( '#ff9900' ); + expect( styles.lineStyles ).toEqual( {} ); + expect( styles.shapeStyles ).toEqual( {} ); + } ); + + it( 'handles DataPointPercentage data without color override', () => { + let contextValue: GlobalChartsContextValue; + + const TestComponent = () => { + contextValue = useGlobalChartsContext(); + return
Test
; + }; + + render( + + + + ); + + const percentageDataWithoutColor = { + label: 'No Color Point', + value: 100, + percentage: 50, + }; + + const styles = contextValue.getElementStyles( { + data: percentageDataWithoutColor, + index: 1, + } ); + + expect( styles.color ).toBe( mockTheme.colors[ 1 ] ); + expect( styles.lineStyles ).toEqual( {} ); + expect( styles.shapeStyles ).toEqual( {} ); + } ); + + it( 'handles DataPointPercentage data with group', () => { + let contextValue: GlobalChartsContextValue; + + const TestComponent = () => { + contextValue = useGlobalChartsContext(); + return
Test
; + }; + + render( + + + + ); + + const percentageDataWithGroup = { + label: 'Grouped Point', + value: 100, + percentage: 50, + group: 'pie-segment-group', + }; + + const styles1 = contextValue.getElementStyles( { + data: percentageDataWithGroup, + index: 0, + } ); + const styles2 = contextValue.getElementStyles( { + data: percentageDataWithGroup, + index: 5, + } ); + + // Should have same color due to group consistency + expect( styles1.color ).toBe( styles2.color ); + } ); + + it( 'prioritizes DataPointPercentage color over group color', () => { + let contextValue: GlobalChartsContextValue; + + const TestComponent = () => { + contextValue = useGlobalChartsContext(); + return
Test
; + }; + + render( + + + + ); + + const percentageDataWithBoth = { + label: 'Both Color and Group', + value: 100, + percentage: 50, + color: '#priority-color', + group: 'test-group', + }; + + const styles = contextValue.getElementStyles( { + data: percentageDataWithBoth, + index: 0, + } ); + + expect( styles.color ).toBe( '#priority-color' ); + } ); + } ); + + describe( 'getElementStyles - SeriesData line and shape styles', () => { + it( 'returns line styles for SeriesData', () => { + let contextValue: GlobalChartsContextValue; + + const TestComponent = () => { + contextValue = useGlobalChartsContext(); + return
Test
; + }; + + const extendedTheme = { + ...mockTheme, + seriesLineStyles: [ { strokeWidth: 2 }, { strokeWidth: 3, strokeDasharray: '2 2' } ], + }; + + render( + + + + ); + + const seriesData = { + label: 'Test Series', + data: [ { value: 100 } ], + options: { + seriesLineStyle: { strokeWidth: 5, strokeDasharray: '10 5' }, + }, + }; + + const styles = contextValue.getElementStyles( { + data: seriesData, + index: 0, + } ); + + expect( styles.lineStyles ).toEqual( { + strokeWidth: 5, + strokeDasharray: '10 5', + } ); + } ); + + it( 'returns shape styles for SeriesData', () => { + let contextValue: GlobalChartsContextValue; + + const TestComponent = () => { + contextValue = useGlobalChartsContext(); + return
Test
; + }; + + const extendedTheme = { + ...mockTheme, + legendShapeStyles: [ + { fill: '#LEGEND1', stroke: '#BORDER1' }, + { fill: '#LEGEND2', strokeWidth: 3 }, + ], + }; + + render( + + + + ); + + const seriesDataWithShapeStyle = { + label: 'Test Series', + data: [ { value: 100 } ], + options: { + legendShapeStyle: { fill: '#CUSTOM', strokeWidth: 5 }, + }, + }; + + const styles = contextValue.getElementStyles( { + data: seriesDataWithShapeStyle, + index: 0, + } ); + + expect( styles.shapeStyles ).toEqual( { + fill: '#CUSTOM', + strokeWidth: 5, + } ); + } ); + + it( 'combines line styles with shape styles when legendShape is line', () => { + let contextValue: GlobalChartsContextValue; + + const TestComponent = () => { + contextValue = useGlobalChartsContext(); + return
Test
; + }; + + const extendedTheme = { + ...mockTheme, + lineChart: { + lineStyles: { + comparison: { + strokeDasharray: '4 4', + strokeLinecap: 'square' as const, + strokeWidth: 1.5, + }, + }, + }, + }; + + render( + + + + ); + + const comparisonSeries = { + label: 'Comparison Series', + data: [ { value: 100 } ], + options: { + type: 'comparison' as const, + legendShapeStyle: { fill: '#CUSTOM' }, + }, + }; + + const styles = contextValue.getElementStyles( { + data: comparisonSeries, + index: 0, + legendShape: 'line', + } ); + + expect( styles.shapeStyles ).toEqual( { + fill: '#CUSTOM', + strokeDasharray: '4 4', + strokeLinecap: 'square', + strokeWidth: 1.5, + } ); + } ); + + it( 'does not include line styles in shapeStyles when legendShape is not line', () => { + let contextValue: GlobalChartsContextValue; + + const TestComponent = () => { + contextValue = useGlobalChartsContext(); + return
Test
; + }; + + const extendedTheme = { + ...mockTheme, + lineChart: { + lineStyles: { + comparison: { + strokeDasharray: '4 4', + strokeLinecap: 'square' as const, + strokeWidth: 1.5, + }, + }, + }, + legendShapeStyles: [ { fill: '#LEGEND1', stroke: '#BORDER1' } ], + }; + + render( + + + + ); + + const comparisonSeries = { + label: 'Comparison Series', + data: [ { value: 100 } ], + options: { + type: 'comparison' as const, + }, + }; + + const styles = contextValue.getElementStyles( { + data: comparisonSeries, + index: 0, + legendShape: 'rect', + } ); + + // Should get theme legend shape styles, not line styles + expect( styles.shapeStyles ).toEqual( { + fill: '#LEGEND1', + stroke: '#BORDER1', + } ); + } ); + } ); + + describe( 'getElementStyles - glyph assignment', () => { + it( 'assigns glyph from theme based on index', () => { + let contextValue: GlobalChartsContextValue; + + const TestComponent = () => { + contextValue = useGlobalChartsContext(); + return
Test
; + }; + + const mockGlyph1 = jest.fn(); + const mockGlyph2 = jest.fn(); + + const themeWithGlyphs = { + ...mockTheme, + glyphs: [ mockGlyph1, mockGlyph2 ], + }; + + render( + + + + ); + + const seriesData = { + label: 'Test Series', + data: [ { value: 100 } ], + }; + + const styles1 = contextValue.getElementStyles( { + data: seriesData, + index: 0, + } ); + const styles2 = contextValue.getElementStyles( { + data: seriesData, + index: 1, + } ); + + expect( styles1.glyph ).toBe( mockGlyph1 ); + expect( styles2.glyph ).toBe( mockGlyph2 ); + } ); + + it( 'handles undefined glyph when index exceeds glyph array', () => { + let contextValue: GlobalChartsContextValue; + + const TestComponent = () => { + contextValue = useGlobalChartsContext(); + return
Test
; + }; + + const mockGlyph = jest.fn(); + const themeWithGlyphs = { + ...mockTheme, + glyphs: [ mockGlyph ], + }; + + render( + + + + ); + + const seriesData = { + label: 'Test Series', + data: [ { value: 100 } ], + }; + + const styles = contextValue.getElementStyles( { + data: seriesData, + index: 5, // Beyond array length + } ); + + expect( styles.glyph ).toBeUndefined(); + } ); + + it( 'handles missing glyphs in theme', () => { + let contextValue: GlobalChartsContextValue; + + const TestComponent = () => { + contextValue = useGlobalChartsContext(); + return
Test
; + }; + + render( + + + + ); + + const seriesData = { + label: 'Test Series', + data: [ { value: 100 } ], + }; + + const styles = contextValue.getElementStyles( { + data: seriesData, + index: 0, + } ); + + expect( styles.glyph ).toBeUndefined(); + } ); + } ); + + describe( 'getElementStyles - complete object structure', () => { + it( 'returns complete ElementStyles object for SeriesData', () => { + let contextValue: GlobalChartsContextValue; + + const TestComponent = () => { + contextValue = useGlobalChartsContext(); + return
Test
; + }; + + const mockGlyph = jest.fn(); + const completeTheme = { + ...mockTheme, + glyphs: [ mockGlyph ], + seriesLineStyles: [ { strokeWidth: 2 } ], + legendShapeStyles: [ { fill: '#SHAPE1' } ], + }; + + render( + + + + ); + + const seriesData = { + label: 'Test Series', + data: [ { value: 100 } ], + group: 'test-group', + }; + + const styles = contextValue.getElementStyles( { + data: seriesData, + index: 0, + } ); + + // Verify all properties are present + expect( styles ).toHaveProperty( 'color' ); + expect( styles ).toHaveProperty( 'lineStyles' ); + expect( styles ).toHaveProperty( 'glyph' ); + expect( styles ).toHaveProperty( 'shapeStyles' ); + + // Verify types + expect( typeof styles.color ).toBe( 'string' ); + expect( typeof styles.lineStyles ).toBe( 'object' ); + expect( typeof styles.glyph ).toBe( 'function' ); + expect( typeof styles.shapeStyles ).toBe( 'object' ); + } ); + + it( 'returns complete ElementStyles object for DataPointPercentage', () => { + let contextValue: GlobalChartsContextValue; + + const TestComponent = () => { + contextValue = useGlobalChartsContext(); + return
Test
; + }; + + const mockGlyph = jest.fn(); + const completeTheme = { + ...mockTheme, + glyphs: [ mockGlyph ], + }; + + render( + + + + ); + + const percentageData = { + label: 'Test Point', + value: 100, + percentage: 50, + color: '#custom', + }; + + const styles = contextValue.getElementStyles( { + data: percentageData, + index: 0, + } ); + + // Verify all properties are present + expect( styles ).toHaveProperty( 'color' ); + expect( styles ).toHaveProperty( 'lineStyles' ); + expect( styles ).toHaveProperty( 'glyph' ); + expect( styles ).toHaveProperty( 'shapeStyles' ); + + // Verify values for percentage data + expect( styles.color ).toBe( '#custom' ); + expect( styles.lineStyles ).toEqual( {} ); + expect( styles.glyph ).toBe( mockGlyph ); + expect( styles.shapeStyles ).toEqual( {} ); + } ); + + it( 'handles undefined data gracefully', () => { + let contextValue: GlobalChartsContextValue; + + const TestComponent = () => { + contextValue = useGlobalChartsContext(); + return
Test
; + }; + + render( + + + + ); + + const styles = contextValue.getElementStyles( { + data: undefined, + index: 0, + } ); + + expect( styles ).toHaveProperty( 'color' ); + expect( styles ).toHaveProperty( 'lineStyles' ); + expect( styles ).toHaveProperty( 'glyph' ); + expect( styles ).toHaveProperty( 'shapeStyles' ); + + expect( styles.color ).toBe( mockTheme.colors[ 0 ] ); + expect( styles.lineStyles ).toEqual( {} ); + expect( styles.shapeStyles ).toEqual( {} ); + } ); + } ); + + describe( 'getElementStyles - overrideColor precedence with SeriesData', () => { + it( 'prioritizes explicit overrideColor over series stroke', () => { + let contextValue: GlobalChartsContextValue; + + const TestComponent = () => { + contextValue = useGlobalChartsContext(); + return
Test
; + }; + + render( + + + + ); + + const seriesWithStroke = { + label: 'Series with stroke', + data: [ { value: 100 } ], + options: { stroke: '#series-stroke' }, + }; + + const styles = contextValue.getElementStyles( { + data: seriesWithStroke, + index: 0, + overrideColor: '#explicit-override', + } ); + + expect( styles.color ).toBe( '#explicit-override' ); + } ); + + it( 'uses series stroke when no explicit overrideColor', () => { + let contextValue: GlobalChartsContextValue; + + const TestComponent = () => { + contextValue = useGlobalChartsContext(); + return
Test
; + }; + + render( + + + + ); + + const seriesWithStroke = { + label: 'Series with stroke', + data: [ { value: 100 } ], + options: { stroke: '#series-stroke' }, + }; + + const styles = contextValue.getElementStyles( { + data: seriesWithStroke, + index: 0, + } ); + + expect( styles.color ).toBe( '#series-stroke' ); + } ); + } ); } ); From a28be0919ba07a36b342567ea4d46a64762fa5c1 Mon Sep 17 00:00:00 2001 From: Adam Wood <1017872+adamwoodnz@users.noreply.github.com> Date: Fri, 19 Sep 2025 11:21:16 +1200 Subject: [PATCH 19/23] Fix legend glyph rendering from theme --- .../legend/hooks/use-chart-legend-items.ts | 56 +++++++++++-------- 1 file changed, 32 insertions(+), 24 deletions(-) diff --git a/projects/js-packages/charts/src/components/legend/hooks/use-chart-legend-items.ts b/projects/js-packages/charts/src/components/legend/hooks/use-chart-legend-items.ts index 6be692f55f117..22d1f85554c05 100644 --- a/projects/js-packages/charts/src/components/legend/hooks/use-chart-legend-items.ts +++ b/projects/js-packages/charts/src/components/legend/hooks/use-chart-legend-items.ts @@ -61,6 +61,36 @@ function formatPointValue( return ''; } +/** + * Applies glyph configuration to a legend item if needed + * @param baseItem - The base legend item + * @param withGlyph - Whether to include glyph rendering + * @param glyph - Glyph component from theme + * @param renderGlyph - Custom glyph render function + * @param glyphSize - Size of the glyph + * @return The legend item with glyph configuration applied if applicable + */ +function applyGlyphToLegendItem( + baseItem: BaseLegendItem, + withGlyph: boolean, + glyph?: < Datum extends object >( props: GlyphProps< Datum > ) => ReactNode, + renderGlyph?: < Datum extends object >( props: GlyphProps< Datum > ) => ReactNode, + glyphSize?: number +): BaseLegendItem { + if ( withGlyph ) { + const glyphToUse = glyph || renderGlyph; + if ( glyphToUse ) { + return { + ...baseItem, + glyphSize, + renderGlyph: glyphToUse, + }; + } + } + + return baseItem; +} + /** * Processes SeriesData into legend items * @param seriesData - The series data to process @@ -95,18 +125,7 @@ function processSeriesData( shapeStyle: shapeStyles, }; - if ( withGlyph ) { - const glyphToUse = renderGlyph || glyph; - if ( glyphToUse ) { - return { - ...baseItem, - glyphSize, - renderGlyph: glyphToUse, - }; - } - } - - return baseItem; + return applyGlyphToLegendItem( baseItem, withGlyph, glyph, renderGlyph, glyphSize ); }; return seriesData.map( mapper ); @@ -148,18 +167,7 @@ function processPointData( shapeStyle: shapeStyles, }; - if ( withGlyph ) { - const glyphToUse = renderGlyph || glyph; - if ( glyphToUse ) { - return { - ...baseItem, - glyphSize, - renderGlyph: glyphToUse, - }; - } - } - - return baseItem; + return applyGlyphToLegendItem( baseItem, withGlyph, glyph, renderGlyph, glyphSize ); }; return pointData.map( mapper ); From 7b83864b1c0921932b1ffe883a157b9b1010cf79 Mon Sep 17 00:00:00 2001 From: Adam Wood <1017872+adamwoodnz@users.noreply.github.com> Date: Fri, 19 Sep 2025 11:21:42 +1200 Subject: [PATCH 20/23] Remove comments from context types --- .../js-packages/charts/src/providers/chart-context/types.ts | 4 ---- 1 file changed, 4 deletions(-) diff --git a/projects/js-packages/charts/src/providers/chart-context/types.ts b/projects/js-packages/charts/src/providers/chart-context/types.ts index 2ee3377ee437b..0daeb1449d7a8 100644 --- a/projects/js-packages/charts/src/providers/chart-context/types.ts +++ b/projects/js-packages/charts/src/providers/chart-context/types.ts @@ -29,10 +29,6 @@ export interface GlobalChartsContextValue { registerChart: ( id: string, data: ChartRegistration ) => void; unregisterChart: ( id: string ) => void; getChartData: ( id: string ) => ChartRegistration | undefined; - /** Theme provided by the GlobalChartsProvider (merged with defaults) */ theme: CompleteChartTheme; - /** - * Get the styles for a series. - */ getElementStyles: ( params: GetElementStylesParams ) => ElementStyles; } From 81a34b8be0004913f958670592c7675484a6c1fc Mon Sep 17 00:00:00 2001 From: Adam Wood <1017872+adamwoodnz@users.noreply.github.com> Date: Fri, 19 Sep 2025 11:26:17 +1200 Subject: [PATCH 21/23] Use getElementStyles for line chart glyphs --- .../charts/src/components/line-chart/line-chart.tsx | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/projects/js-packages/charts/src/components/line-chart/line-chart.tsx b/projects/js-packages/charts/src/components/line-chart/line-chart.tsx index 507043ad5d4cd..ef97b87d3ba12 100644 --- a/projects/js-packages/charts/src/components/line-chart/line-chart.tsx +++ b/projects/js-packages/charts/src/components/line-chart/line-chart.tsx @@ -419,7 +419,7 @@ const LineChartInternal = forwardRef< SingleChartRef, LineChartProps >( { dataSorted.map( ( seriesData, index ) => { - const { color, lineStyles } = getElementStyles( { + const { color, lineStyles, glyph } = getElementStyles( { data: seriesData, index, } ); @@ -436,7 +436,7 @@ const LineChartInternal = forwardRef< SingleChartRef, LineChartProps >( index={ index } data={ seriesData } color={ color } - renderGlyph={ providerTheme.glyphs?.[ index ] ?? renderGlyph } + renderGlyph={ glyph ?? renderGlyph } accessors={ accessors } glyphStyle={ glyphStyle } /> From 23af8fa256db9a770f3b81782d7c36e3354b0ed3 Mon Sep 17 00:00:00 2001 From: Adam Wood <1017872+adamwoodnz@users.noreply.github.com> Date: Fri, 19 Sep 2025 12:05:27 +1200 Subject: [PATCH 22/23] Update test suite description --- .../src/providers/chart-context/test/chart-context.test.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/projects/js-packages/charts/src/providers/chart-context/test/chart-context.test.tsx b/projects/js-packages/charts/src/providers/chart-context/test/chart-context.test.tsx index 67ee0272aba44..40c19d5744b6f 100644 --- a/projects/js-packages/charts/src/providers/chart-context/test/chart-context.test.tsx +++ b/projects/js-packages/charts/src/providers/chart-context/test/chart-context.test.tsx @@ -224,7 +224,7 @@ describe( 'ChartContext', () => { } ); } ); - describe( 'Group Color Resolver', () => { + describe( 'Color resolution', () => { it( 'provides getElementStyles function for color resolution', () => { let contextValue: GlobalChartsContextValue; From 4528ea53bb1f9a4c3b470ad432ff7e7a02857c7c Mon Sep 17 00:00:00 2001 From: Adam Wood <1017872+adamwoodnz@users.noreply.github.com> Date: Fri, 19 Sep 2025 12:15:54 +1200 Subject: [PATCH 23/23] Update test --- .../src/providers/chart-context/test/chart-context.test.tsx | 1 + 1 file changed, 1 insertion(+) diff --git a/projects/js-packages/charts/src/providers/chart-context/test/chart-context.test.tsx b/projects/js-packages/charts/src/providers/chart-context/test/chart-context.test.tsx index 40c19d5744b6f..13ed9a42724fc 100644 --- a/projects/js-packages/charts/src/providers/chart-context/test/chart-context.test.tsx +++ b/projects/js-packages/charts/src/providers/chart-context/test/chart-context.test.tsx @@ -1194,6 +1194,7 @@ describe( 'ChartContext', () => { expect( styles.color ).toBe( mockTheme.colors[ 0 ] ); expect( styles.lineStyles ).toEqual( {} ); + expect( styles.glyph ).toBeUndefined(); expect( styles.shapeStyles ).toEqual( {} ); } ); } );