From f5a9293dc93539067b65ec58cbc4ce01e69de8cb Mon Sep 17 00:00:00 2001 From: Adam Wood <1017872+adamwoodnz@users.noreply.github.com> Date: Thu, 12 Mar 2026 12:55:14 +1300 Subject: [PATCH 01/37] Charts: Add ChartLayout component for shared chart+legend layout Introduces a ChartLayout component that encapsulates the shared Stack + legend positioning pattern used by all chart components. Includes comprehensive tests covering legend positioning, visibility management, ref forwarding, and composition API slot rendering. Co-Authored-By: Claude Opus 4.6 --- .../chart-layout/chart-layout.module.scss | 5 + .../components/chart-layout/chart-layout.tsx | 82 +++++++++ .../src/components/chart-layout/index.ts | 1 + .../chart-layout/test/chart-layout.test.tsx | 171 ++++++++++++++++++ 4 files changed, 259 insertions(+) create mode 100644 projects/js-packages/charts/src/components/chart-layout/chart-layout.module.scss create mode 100644 projects/js-packages/charts/src/components/chart-layout/chart-layout.tsx create mode 100644 projects/js-packages/charts/src/components/chart-layout/index.ts create mode 100644 projects/js-packages/charts/src/components/chart-layout/test/chart-layout.test.tsx diff --git a/projects/js-packages/charts/src/components/chart-layout/chart-layout.module.scss b/projects/js-packages/charts/src/components/chart-layout/chart-layout.module.scss new file mode 100644 index 000000000000..0f3fd3c9a081 --- /dev/null +++ b/projects/js-packages/charts/src/components/chart-layout/chart-layout.module.scss @@ -0,0 +1,5 @@ +// Intentionally minimal — ChartLayout is a structural wrapper. +// Chart-specific styles remain in each chart's own SCSS module. +.chart-layout { + // Base styles if needed in future. Currently the Stack handles layout. +} diff --git a/projects/js-packages/charts/src/components/chart-layout/chart-layout.tsx b/projects/js-packages/charts/src/components/chart-layout/chart-layout.tsx new file mode 100644 index 000000000000..ec03b91691d6 --- /dev/null +++ b/projects/js-packages/charts/src/components/chart-layout/chart-layout.tsx @@ -0,0 +1,82 @@ +import { Stack } from '@wordpress/ui'; +import clsx from 'clsx'; +import { forwardRef } from 'react'; +import { renderLegendSlot } from '../../charts/private/chart-composition'; +import styles from './chart-layout.module.scss'; +import type { LegendChild } from '../../charts/private/chart-composition/use-chart-children'; +import type { LegendPosition } from '../../types'; +import type { GapSize } from '@wordpress/theme'; +import type { CSSProperties, ReactNode } from 'react'; + +interface ChartLayoutProps { + /** Position for the prop-based legend element */ + legendPosition: LegendPosition; + /** The legend element rendered via the showLegend prop (false when hidden) */ + legendElement?: ReactNode; + /** Legend children from the composition API */ + legendChildren: LegendChild[]; + /** Chart content rendered between legend slots */ + children: ReactNode; + /** Content rendered after the bottom legend (e.g., nonLegendChildren, htmlChildren, tooltips) */ + trailingContent?: ReactNode; + /** When true, sets visibility: hidden on the container. Used by charts that need measurement before rendering. */ + isWaitingForMeasurement?: boolean; + /** Gap between Stack items */ + gap?: GapSize; + /** Additional class names */ + className?: string; + /** Inline styles (width, height, etc.) */ + style?: CSSProperties; + /** Test ID for the container */ + 'data-testid'?: string; + /** Chart ID attribute */ + 'data-chart-id'?: string; +} + +export const ChartLayout = forwardRef< HTMLDivElement, ChartLayoutProps >( + ( + { + legendPosition, + legendElement, + legendChildren, + children, + trailingContent, + isWaitingForMeasurement, + gap, + className, + style, + 'data-testid': dataTestId, + 'data-chart-id': dataChartId, + }, + ref + ) => { + const visibilityStyle = + isWaitingForMeasurement !== undefined + ? { visibility: ( isWaitingForMeasurement ? 'hidden' : 'visible' ) as const } + : {}; + + return ( + + { legendPosition === 'top' && legendElement } + { renderLegendSlot( legendChildren, 'top' ) } + + { children } + + { legendPosition === 'bottom' && legendElement } + { renderLegendSlot( legendChildren, 'bottom' ) } + + { trailingContent } + + ); + } +); + +ChartLayout.displayName = 'ChartLayout'; diff --git a/projects/js-packages/charts/src/components/chart-layout/index.ts b/projects/js-packages/charts/src/components/chart-layout/index.ts new file mode 100644 index 000000000000..077106657634 --- /dev/null +++ b/projects/js-packages/charts/src/components/chart-layout/index.ts @@ -0,0 +1 @@ +export { ChartLayout } from './chart-layout'; diff --git a/projects/js-packages/charts/src/components/chart-layout/test/chart-layout.test.tsx b/projects/js-packages/charts/src/components/chart-layout/test/chart-layout.test.tsx new file mode 100644 index 000000000000..4d53e9343703 --- /dev/null +++ b/projects/js-packages/charts/src/components/chart-layout/test/chart-layout.test.tsx @@ -0,0 +1,171 @@ +/* eslint-disable no-bitwise, jest/expect-expect */ +import { render, screen } from '@testing-library/react'; +import { renderLegendSlot } from '../../../charts/private/chart-composition'; +import { ChartLayout } from '../chart-layout'; +import type { LegendChild } from '../../../charts/private/chart-composition/use-chart-children'; + +// Mock renderLegendSlot since we test it separately +jest.mock( '../../../charts/private/chart-composition', () => ( { + renderLegendSlot: jest.fn( () => [] ), +} ) ); + +const mockRenderLegendSlot = renderLegendSlot as jest.Mock; + +/** + * Asserts that `earlier` appears before `later` in DOM order. + * + * @param {HTMLElement} earlier - Element expected to appear first + * @param {HTMLElement} later - Element expected to appear second + */ +function expectDomOrder( earlier: HTMLElement, later: HTMLElement ) { + expect( + earlier.compareDocumentPosition( later ) & Node.DOCUMENT_POSITION_FOLLOWING + ).toBeTruthy(); +} + +describe( 'ChartLayout', () => { + beforeEach( () => { + mockRenderLegendSlot.mockReturnValue( [] ); + } ); + + it( 'renders children inside a column Stack', () => { + render( + +
Chart
+ + ); + expect( screen.getByTestId( 'chart-content' ) ).toBeInTheDocument(); + } ); + + it( 'renders legend element at top when legendPosition is top', () => { + const legendElement =
Legend
; + render( + +
Chart
+ + ); + // Legend should come before content in DOM order + expectDomOrder( screen.getByTestId( 'legend' ), screen.getByTestId( 'chart-content' ) ); + } ); + + it( 'renders legend element at bottom when legendPosition is bottom', () => { + const legendElement =
Legend
; + render( + +
Chart
+ + ); + // Content should come before legend in DOM order + expectDomOrder( screen.getByTestId( 'chart-content' ), screen.getByTestId( 'legend' ) ); + } ); + + it( 'does not render legend element when it is false/null', () => { + render( + +
Chart
+ + ); + expect( screen.queryByTestId( 'legend' ) ).not.toBeInTheDocument(); + } ); + + it( 'calls renderLegendSlot for both positions', () => { + const legendChildren: LegendChild[] = []; + render( + +
Chart
+ + ); + expect( mockRenderLegendSlot ).toHaveBeenCalledWith( legendChildren, 'top' ); + expect( mockRenderLegendSlot ).toHaveBeenCalledWith( legendChildren, 'bottom' ); + } ); + + it( 'applies visibility hidden when isWaitingForMeasurement is true', () => { + render( + +
Chart
+ + ); + expect( screen.getByTestId( 'layout' ) ).toHaveStyle( { visibility: 'hidden' } ); + } ); + + it( 'applies visibility visible when isWaitingForMeasurement is false', () => { + render( + +
Chart
+ + ); + expect( screen.getByTestId( 'layout' ) ).toHaveStyle( { visibility: 'visible' } ); + } ); + + it( 'does not set visibility in inline style when isWaitingForMeasurement is undefined', () => { + render( + +
Chart
+ + ); + // When isWaitingForMeasurement is not provided, visibility should not be set in inline style + const layoutStyle = screen.getByTestId( 'layout' ).getAttribute( 'style' ) ?? ''; + expect( layoutStyle ).not.toContain( 'visibility' ); + } ); + + it( 'passes className and style to Stack', () => { + render( + +
Chart
+ + ); + const layout = screen.getByTestId( 'layout' ); + expect( layout ).toHaveClass( 'my-chart' ); + expect( layout ).toHaveStyle( { width: '400px', height: '300px' } ); + } ); + + it( 'passes gap to Stack', () => { + render( + +
Chart
+ + ); + // Stack renders gap as a CSS class or style — just verify it renders without error + expect( screen.getByTestId( 'layout' ) ).toBeInTheDocument(); + } ); + + it( 'forwards ref to Stack', () => { + const ref = jest.fn(); + render( + +
Chart
+ + ); + expect( ref ).toHaveBeenCalledWith( expect.any( HTMLElement ) ); + } ); + + it( 'renders trailing content after bottom legend', () => { + render( + Legend } + legendChildren={ [] } + trailingContent={
Extra
} + > +
Chart
+ + ); + expectDomOrder( screen.getByTestId( 'legend' ), screen.getByTestId( 'trailing' ) ); + } ); +} ); From e621a42627c7f362c160bcedafad50176eb3543b Mon Sep 17 00:00:00 2001 From: Adam Wood <1017872+adamwoodnz@users.noreply.github.com> Date: Thu, 12 Mar 2026 12:58:56 +1300 Subject: [PATCH 02/37] Charts: Export ChartLayoutProps type from barrel Co-Authored-By: Claude Opus 4.6 --- .../charts/src/components/chart-layout/chart-layout.tsx | 2 +- .../js-packages/charts/src/components/chart-layout/index.ts | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/projects/js-packages/charts/src/components/chart-layout/chart-layout.tsx b/projects/js-packages/charts/src/components/chart-layout/chart-layout.tsx index ec03b91691d6..a0da71fa6f40 100644 --- a/projects/js-packages/charts/src/components/chart-layout/chart-layout.tsx +++ b/projects/js-packages/charts/src/components/chart-layout/chart-layout.tsx @@ -8,7 +8,7 @@ import type { LegendPosition } from '../../types'; import type { GapSize } from '@wordpress/theme'; import type { CSSProperties, ReactNode } from 'react'; -interface ChartLayoutProps { +export interface ChartLayoutProps { /** Position for the prop-based legend element */ legendPosition: LegendPosition; /** The legend element rendered via the showLegend prop (false when hidden) */ diff --git a/projects/js-packages/charts/src/components/chart-layout/index.ts b/projects/js-packages/charts/src/components/chart-layout/index.ts index 077106657634..8e997d8803af 100644 --- a/projects/js-packages/charts/src/components/chart-layout/index.ts +++ b/projects/js-packages/charts/src/components/chart-layout/index.ts @@ -1 +1,2 @@ export { ChartLayout } from './chart-layout'; +export type { ChartLayoutProps } from './chart-layout'; From 38db8be1f2f04d7d314fec508af95935a573dcec Mon Sep 17 00:00:00 2001 From: Adam Wood <1017872+adamwoodnz@users.noreply.github.com> Date: Thu, 12 Mar 2026 13:01:14 +1300 Subject: [PATCH 03/37] Charts: Migrate LineChart to use ChartLayout Co-Authored-By: Claude Opus 4.6 --- .../src/charts/line-chart/line-chart.tsx | 28 +++++++------------ 1 file changed, 10 insertions(+), 18 deletions(-) diff --git a/projects/js-packages/charts/src/charts/line-chart/line-chart.tsx b/projects/js-packages/charts/src/charts/line-chart/line-chart.tsx index 8a800e2563a6..c9b949d03cdf 100644 --- a/projects/js-packages/charts/src/charts/line-chart/line-chart.tsx +++ b/projects/js-packages/charts/src/charts/line-chart/line-chart.tsx @@ -4,10 +4,10 @@ import { LinearGradient } from '@visx/gradient'; import { scaleTime } from '@visx/scale'; import { XYChart, AreaSeries, Grid, Axis, DataContext } from '@visx/xychart'; import { __ } from '@wordpress/i18n'; -import { Stack } from '@wordpress/ui'; import clsx from 'clsx'; import { differenceInHours, differenceInYears } from 'date-fns'; import { useMemo, useContext, forwardRef, useImperativeHandle, useState, useRef } from 'react'; +import { ChartLayout } from '../../components/chart-layout'; import { Legend, useChartLegendItems } from '../../components/legend'; import { AccessibleTooltip, useKeyboardNavigation } from '../../components/tooltip'; import { @@ -26,7 +26,7 @@ import { useGlobalChartsTheme, } from '../../providers'; import { attachSubComponents } from '../../utils'; -import { useChartChildren, renderLegendSlot } from '../private/chart-composition'; +import { useChartChildren } from '../private/chart-composition'; import { DefaultGlyph } from '../private/default-glyph'; import { SingleChartContext, type SingleChartRef } from '../private/single-chart-context'; import { withResponsive } from '../private/with-responsive'; @@ -475,8 +475,11 @@ const LineChartInternal = forwardRef< SingleChartRef, LineChartProps >( chartHeight, } } > - ( { [ styles[ 'line-chart--animated' ] ]: animation && ! prefersReducedMotion }, className ) } + style={ { width, height } } data-testid="line-chart" - style={ { - width, - height, - visibility: isWaitingForMeasurement ? 'hidden' : 'visible', - } } + trailingContent={ nonLegendChildren } > - { legendPosition === 'top' && legendElement } - { renderLegendSlot( legendChildren, 'top' ) } -
(
) } - - { legendPosition === 'bottom' && legendElement } - { renderLegendSlot( legendChildren, 'bottom' ) } - - { nonLegendChildren } - + ); } From 8b757a7c120eb24a936dac89afb53bb5c625424f Mon Sep 17 00:00:00 2001 From: Adam Wood <1017872+adamwoodnz@users.noreply.github.com> Date: Thu, 12 Mar 2026 13:03:39 +1300 Subject: [PATCH 04/37] Charts: Migrate BarChart to use ChartLayout Co-Authored-By: Claude Opus 4.6 --- .../charts/src/charts/bar-chart/bar-chart.tsx | 28 +++++++------------ 1 file changed, 10 insertions(+), 18 deletions(-) diff --git a/projects/js-packages/charts/src/charts/bar-chart/bar-chart.tsx b/projects/js-packages/charts/src/charts/bar-chart/bar-chart.tsx index 478843f3413d..39ac9805ae15 100644 --- a/projects/js-packages/charts/src/charts/bar-chart/bar-chart.tsx +++ b/projects/js-packages/charts/src/charts/bar-chart/bar-chart.tsx @@ -2,9 +2,9 @@ import { formatNumber } from '@automattic/number-formatters'; import { PatternLines, PatternCircles, PatternWaves, PatternHexagons } from '@visx/pattern'; import { Axis, BarSeries, BarGroup, Grid, XYChart } from '@visx/xychart'; import { __ } from '@wordpress/i18n'; -import { Stack } from '@wordpress/ui'; import clsx from 'clsx'; import { useCallback, useContext, useState, useRef, useMemo } from 'react'; +import { ChartLayout } from '../../components/chart-layout'; import { Legend, useChartLegendItems } from '../../components/legend'; import { AccessibleTooltip, useKeyboardNavigation } from '../../components/tooltip'; import { @@ -24,7 +24,7 @@ import { GlobalChartsContext, } from '../../providers'; import { attachSubComponents } from '../../utils'; -import { useChartChildren, renderLegendSlot } from '../private/chart-composition'; +import { useChartChildren } from '../private/chart-composition'; import { SingleChartContext } from '../private/single-chart-context'; import { withResponsive } from '../private/with-responsive'; import styles from './bar-chart.module.scss'; @@ -346,8 +346,11 @@ const BarChartInternal: FC< BarChartProps > = ( { chartHeight, } } > - = ( { }, className ) } + style={ { width, height } } data-testid="bar-chart" - style={ { - width, - height, - visibility: isWaitingForMeasurement ? 'hidden' : 'visible', - } } data-chart-id={ `bar-chart-${ chartId }` } + trailingContent={ nonLegendChildren } > - { legendPosition === 'top' && legendElement } - { renderLegendSlot( legendChildren, 'top' ) } -
= ( {
) } - - { legendPosition === 'bottom' && legendElement } - { renderLegendSlot( legendChildren, 'bottom' ) } - - { nonLegendChildren } - + ); }; From 6125f8b80bf21d2b79c540ae1630a9ceb3bf843e Mon Sep 17 00:00:00 2001 From: Adam Wood <1017872+adamwoodnz@users.noreply.github.com> Date: Thu, 12 Mar 2026 13:05:56 +1300 Subject: [PATCH 05/37] Charts: Migrate PieChart to use ChartLayout Co-Authored-By: Claude Opus 4.6 --- .../charts/src/charts/pie-chart/pie-chart.tsx | 46 ++++++++----------- 1 file changed, 18 insertions(+), 28 deletions(-) diff --git a/projects/js-packages/charts/src/charts/pie-chart/pie-chart.tsx b/projects/js-packages/charts/src/charts/pie-chart/pie-chart.tsx index a81426bc596f..186a92bd44a0 100644 --- a/projects/js-packages/charts/src/charts/pie-chart/pie-chart.tsx +++ b/projects/js-packages/charts/src/charts/pie-chart/pie-chart.tsx @@ -2,9 +2,9 @@ import { Group } from '@visx/group'; import { Pie } from '@visx/shape'; import { useTooltip, useTooltipInPortal } from '@visx/tooltip'; import { __ } from '@wordpress/i18n'; -import { Stack } from '@wordpress/ui'; import clsx from 'clsx'; import { useCallback, useContext, useMemo } from 'react'; +import { ChartLayout } from '../../components/chart-layout'; import { Legend, useChartLegendItems } from '../../components/legend'; import { BaseTooltip } from '../../components/tooltip'; import { useElementSize, useInteractiveLegendData, usePrefersReducedMotion } from '../../hooks'; @@ -18,12 +18,7 @@ import { } from '../../providers'; import { attachSubComponents } from '../../utils'; import { getStringWidth } from '../../visx/text'; -import { - ChartSVG, - ChartHTML, - useChartChildren, - renderLegendSlot, -} from '../private/chart-composition'; +import { ChartSVG, ChartHTML, useChartChildren } from '../private/chart-composition'; import { RadialWipeAnimation } from '../private/radial-wipe-animation/'; import { SingleChartContext } from '../private/single-chart-context'; import { withResponsive, ResponsiveConfig } from '../private/with-responsive'; @@ -332,9 +327,11 @@ const PieChartInternal = ( { chartHeight: height, } } > - + { withTooltips && tooltipOpen && tooltipData && ( + +
{ renderTooltip( { tooltipData } ) }
+ + ) } + { htmlChildren } + { otherChildren } + + } > - { legendPosition === 'top' && legendElement } - { renderLegendSlot( legendChildren, 'top' ) } -
- - { legendPosition === 'bottom' && legendElement } - { renderLegendSlot( legendChildren, 'bottom' ) } - - { withTooltips && tooltipOpen && tooltipData && ( - -
{ renderTooltip( { tooltipData } ) }
- - ) } - - { /* Render HTML component children from PieChart.HTML */ } - { htmlChildren } - - { /* Render other React children for backward compatibility */ } - { otherChildren } - + ); }; From db08da860ff7913510b3a85e601ef45a3c894145 Mon Sep 17 00:00:00 2001 From: Adam Wood <1017872+adamwoodnz@users.noreply.github.com> Date: Thu, 12 Mar 2026 13:08:47 +1300 Subject: [PATCH 06/37] Charts: Migrate PieSemiCircleChart to use ChartLayout Co-Authored-By: Claude Opus 4.6 --- .../pie-semi-circle-chart.tsx | 46 ++++++++----------- 1 file changed, 18 insertions(+), 28 deletions(-) diff --git a/projects/js-packages/charts/src/charts/pie-semi-circle-chart/pie-semi-circle-chart.tsx b/projects/js-packages/charts/src/charts/pie-semi-circle-chart/pie-semi-circle-chart.tsx index 7835241e5415..3583b5554dc7 100644 --- a/projects/js-packages/charts/src/charts/pie-semi-circle-chart/pie-semi-circle-chart.tsx +++ b/projects/js-packages/charts/src/charts/pie-semi-circle-chart/pie-semi-circle-chart.tsx @@ -3,9 +3,9 @@ import { Pie } from '@visx/shape'; import { Text } from '@visx/text'; import { useTooltip, useTooltipInPortal } from '@visx/tooltip'; import { __ } from '@wordpress/i18n'; -import { Stack } from '@wordpress/ui'; import clsx from 'clsx'; import { useCallback, useContext, useMemo } from 'react'; +import { ChartLayout } from '../../components/chart-layout'; import { Legend, useChartLegendItems } from '../../components/legend'; import { BaseTooltip } from '../../components/tooltip'; import { useElementSize, useInteractiveLegendData, usePrefersReducedMotion } from '../../hooks'; @@ -17,12 +17,7 @@ import { GlobalChartsContext, } from '../../providers'; import { attachSubComponents } from '../../utils'; -import { - ChartSVG, - ChartHTML, - useChartChildren, - renderLegendSlot, -} from '../private/chart-composition'; +import { ChartSVG, ChartHTML, useChartChildren } from '../private/chart-composition'; import { RadialWipeAnimation } from '../private/radial-wipe-animation'; import { SingleChartContext } from '../private/single-chart-context'; import { withResponsive } from '../private/with-responsive'; @@ -364,9 +359,11 @@ const PieSemiCircleChartInternal: FC< PieSemiCircleChartProps > = ( { chartHeight: height, } } > - = ( { height: propHeight || undefined, } } data-testid="pie-chart-container" + trailingContent={ + <> + { withTooltips && tooltipOpen && tooltipData && ( + +
{ renderTooltip( { tooltipData } ) }
+ + ) } + { htmlChildren } + { otherChildren } + + } > - { legendPosition === 'top' && legendElement } - { renderLegendSlot( legendChildren, 'top' ) } -
= ( {
- - { legendPosition === 'bottom' && legendElement } - { renderLegendSlot( legendChildren, 'bottom' ) } - - { withTooltips && tooltipOpen && tooltipData && ( - -
{ renderTooltip( { tooltipData } ) }
- - ) } - - { /* Render HTML children from composition API */ } - { htmlChildren } - - { /* Render any other children that aren't compound components */ } - { otherChildren } - + ); }; From 8bda08ec812f2453f9acdacbeebaf339a7efe8b2 Mon Sep 17 00:00:00 2001 From: Adam Wood <1017872+adamwoodnz@users.noreply.github.com> Date: Thu, 12 Mar 2026 13:11:05 +1300 Subject: [PATCH 07/37] Charts: Migrate LeaderboardChart to use ChartLayout Co-Authored-By: Claude Opus 4.6 --- .../leaderboard-chart/leaderboard-chart.tsx | 64 ++++++------------- 1 file changed, 20 insertions(+), 44 deletions(-) diff --git a/projects/js-packages/charts/src/charts/leaderboard-chart/leaderboard-chart.tsx b/projects/js-packages/charts/src/charts/leaderboard-chart/leaderboard-chart.tsx index 53442550be89..9ce06d42a292 100644 --- a/projects/js-packages/charts/src/charts/leaderboard-chart/leaderboard-chart.tsx +++ b/projects/js-packages/charts/src/charts/leaderboard-chart/leaderboard-chart.tsx @@ -5,6 +5,7 @@ import { __ } from '@wordpress/i18n'; import { Stack } from '@wordpress/ui'; import clsx from 'clsx'; import { useContext, useMemo, type FC } from 'react'; +import { ChartLayout } from '../../components/chart-layout'; import { Legend } from '../../components/legend'; import { usePrefersReducedMotion } from '../../hooks'; import { @@ -16,7 +17,7 @@ import { useGlobalChartsTheme, } from '../../providers'; import { formatMetricValue, attachSubComponents } from '../../utils'; -import { useChartChildren, renderLegendSlot } from '../private/chart-composition'; +import { useChartChildren } from '../private/chart-composition'; import { SingleChartContext } from '../private/single-chart-context'; import { withResponsive } from '../private/with-responsive'; import { useLeaderboardLegendItems } from './hooks'; @@ -246,37 +247,23 @@ const LeaderboardChartInternal: FC< LeaderboardChartProps > = ( { // Handle empty or undefined data if ( ! data || data.length === 0 ) { return ( - - +
{ loading ? __( 'Loading…', 'jetpack-charts' ) : __( 'No data available', 'jetpack-charts' ) }
- - { nonLegendChildren } - + ); } @@ -297,16 +284,11 @@ const LeaderboardChartInternal: FC< LeaderboardChartProps > = ( { ); return ( - - + = ( { width: propWidth || undefined, height: propHeight || undefined, } } + data-testid="leaderboard-chart-container" + trailingContent={ nonLegendChildren } > - { legendPosition === 'top' && legendElement } - { renderLegendSlot( legendChildren, 'top' ) } -
{ allSeriesHidden ? (
@@ -372,12 +353,7 @@ const LeaderboardChartInternal: FC< LeaderboardChartProps > = ( { ) }
- - { legendPosition === 'bottom' && legendElement } - { renderLegendSlot( legendChildren, 'bottom' ) } - - { nonLegendChildren } - + ); }; From 51d2eabd018d3028648d4b39c109557cd734d349 Mon Sep 17 00:00:00 2001 From: Adam Wood <1017872+adamwoodnz@users.noreply.github.com> Date: Thu, 12 Mar 2026 13:14:34 +1300 Subject: [PATCH 08/37] Charts: Restore responsive/loading classes in LeaderboardChart empty state Co-Authored-By: Claude Opus 4.6 --- .../src/charts/leaderboard-chart/leaderboard-chart.tsx | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/projects/js-packages/charts/src/charts/leaderboard-chart/leaderboard-chart.tsx b/projects/js-packages/charts/src/charts/leaderboard-chart/leaderboard-chart.tsx index 9ce06d42a292..408052d70adb 100644 --- a/projects/js-packages/charts/src/charts/leaderboard-chart/leaderboard-chart.tsx +++ b/projects/js-packages/charts/src/charts/leaderboard-chart/leaderboard-chart.tsx @@ -252,7 +252,14 @@ const LeaderboardChartInternal: FC< LeaderboardChartProps > = ( { legendPosition={ legendPosition } legendElement={ false } legendChildren={ legendChildren } - className={ clsx( styles.leaderboardChart, className ) } + className={ clsx( + styles.leaderboardChart, + { + [ styles[ 'leaderboardChart--responsive' ] ]: ! propWidth && ! propHeight, + [ styles[ 'leaderboardChart--loading' ] ]: loading, + }, + className + ) } gap={ gap } style={ { ...style, width: propWidth || undefined, height: propHeight || undefined } } data-testid="leaderboard-chart-container" From 504ea453fd62c8aa77df894daecc425c04bb7797 Mon Sep 17 00:00:00 2001 From: Adam Wood <1017872+adamwoodnz@users.noreply.github.com> Date: Thu, 12 Mar 2026 14:31:49 +1300 Subject: [PATCH 09/37] Charts: PieChart uses ChartLayout render prop for measurement Co-Authored-By: Claude Opus 4.6 --- .../charts/src/charts/pie-chart/pie-chart.tsx | 356 ++++++++++-------- 1 file changed, 194 insertions(+), 162 deletions(-) diff --git a/projects/js-packages/charts/src/charts/pie-chart/pie-chart.tsx b/projects/js-packages/charts/src/charts/pie-chart/pie-chart.tsx index 186a92bd44a0..95051e1808a3 100644 --- a/projects/js-packages/charts/src/charts/pie-chart/pie-chart.tsx +++ b/projects/js-packages/charts/src/charts/pie-chart/pie-chart.tsx @@ -7,7 +7,7 @@ import { useCallback, useContext, useMemo } from 'react'; import { ChartLayout } from '../../components/chart-layout'; import { Legend, useChartLegendItems } from '../../components/legend'; import { BaseTooltip } from '../../components/tooltip'; -import { useElementSize, useInteractiveLegendData, usePrefersReducedMotion } from '../../hooks'; +import { useInteractiveLegendData, usePrefersReducedMotion } from '../../hooks'; import { GlobalChartsProvider, useChartId, @@ -184,7 +184,6 @@ const PieChartInternal = ( { const providerTheme = useGlobalChartsTheme(); const chartId = useChartId( providedChartId ); - const [ svgWrapperRef, svgWrapperWidth, svgWrapperHeight ] = useElementSize< HTMLDivElement >(); const { tooltipOpen, tooltipLeft, tooltipTop, tooltipData, hideTooltip, showTooltip } = useTooltip< DataPointPercentage >(); @@ -259,34 +258,9 @@ const PieChartInternal = ( { ); } - // Calculate the actual pie size: - // - Measure available space from the svg-wrapper - // - If size prop provided: use it as max, but shrink if container is smaller - // - If no size prop: fill available space - const availableWidth = svgWrapperWidth > 0 ? svgWrapperWidth : 300; - const availableHeight = svgWrapperHeight > 0 ? svgWrapperHeight : 300; - const availableSize = Math.min( availableWidth, availableHeight ); - const actualSize = size ? Math.min( size, availableSize ) : availableSize; - - const width = actualSize; - const height = actualSize; - - // Calculate radius based on width/height - const radius = Math.min( width, height ) / 2; - - // Center the chart in the available space - const centerX = width / 2; - const centerY = height / 2; - // Calculate the angle between each (use original data length for consistent spacing) const padAngle = gapScale * ( ( 2 * Math.PI ) / data.length ); - const outerRadius = radius - padding; - const innerRadius = thickness === 0 ? 0 : outerRadius * ( 1 - thickness ); - - const maxCornerRadius = ( outerRadius - innerRadius ) / 2; - const cornerRadius = cornerScale ? Math.min( cornerScale * outerRadius, maxCornerRadius ) : 0; - // Map the data to include index for color assignment // When interactive, we need to find the original index to maintain consistent colors const dataWithIndex = visibleData.map( d => { @@ -320,15 +294,8 @@ const PieChartInternal = ( { ); return ( - - } > -
- - - - - - { + const availableWidth = contentWidth > 0 ? contentWidth : 300; + const availableHeight = contentHeight > 0 ? contentHeight : 300; + const availableSize = Math.min( availableWidth, availableHeight ); + const actualSize = size ? Math.min( size, availableSize ) : availableSize; + + const width = actualSize; + const height = actualSize; + + const radius = Math.min( width, height ) / 2; + const centerX = width / 2; + const centerY = height / 2; + + const outerRadius = radius - padding; + const innerRadius = thickness === 0 ? 0 : outerRadius * ( 1 - thickness ); + + const maxCornerRadius = ( outerRadius - innerRadius ) / 2; + const cornerRadius = cornerScale + ? Math.min( cornerScale * outerRadius, maxCornerRadius ) + : 0; + + return ( + - { allSegmentsHidden ? ( - + + + + + + - { __( - 'All segments are hidden. Click legend items to show data.', - 'jetpack-charts' - ) } - - ) : ( - - data={ dataWithIndex } - pieValue={ accessors.value } - outerRadius={ outerRadius } - innerRadius={ innerRadius } - padAngle={ padAngle } - cornerRadius={ cornerRadius } - > - { pie => { - return pie.arcs.map( ( arc, index ) => { - const [ centroidX, centroidY ] = pie.path.centroid( arc ); - const hasSpaceForLabel = arc.endAngle - arc.startAngle >= 0.25; - const handleMouseMove = ( event: MouseEvent< SVGElement > ) => { - if ( ! withTooltips ) { - return; - } - - // Don't show tooltip until container bounds are measured - if ( containerBounds.width === 0 || containerBounds.height === 0 ) { - return; - } - - // Use clientX/Y and subtract containerBounds to cancel out any stale offset. - // TooltipInPortal calculates: tooltipLeft + containerBounds.left + scrollX - // By passing (clientX - containerBounds.left), we get: - // (clientX - containerBounds.left) + containerBounds.left + scrollX = clientX + scrollX - // This gives correct page coordinates regardless of stale bounds. - showTooltip( { - tooltipData: arc.data, - tooltipLeft: event.clientX - containerBounds.left + tooltipOffsetX, - tooltipTop: event.clientY - containerBounds.top + tooltipOffsetY, - } ); - }; - - const pathProps: SVGProps< SVGPathElement > & { 'data-testid'?: string } = { - d: pie.path( arc ) || '', - fill: accessors.fill( arc.data ), - 'data-testid': 'pie-segment', - }; - - const groupProps: SVGProps< SVGGElement > = {}; - if ( withTooltips ) { - groupProps.onMouseMove = handleMouseMove; - groupProps.onMouseLeave = onMouseLeave; - } - - // Estimate text width more accurately for background sizing - const fontSize = 12; - const estimatedTextWidth = getStringWidth( arc.data.label, { fontSize } ); - const labelPadding = 6; - const backgroundWidth = estimatedTextWidth + labelPadding * 2; - const backgroundHeight = fontSize + labelPadding * 2; - - return ( - - - { showLabels && hasSpaceForLabel && ( - - { providerTheme.labelBackgroundColor && ( - + { allSegmentsHidden ? ( + + { __( + 'All segments are hidden. Click legend items to show data.', + 'jetpack-charts' + ) } + + ) : ( + + data={ dataWithIndex } + pieValue={ accessors.value } + outerRadius={ outerRadius } + innerRadius={ innerRadius } + padAngle={ padAngle } + cornerRadius={ cornerRadius } + > + { pie => { + return pie.arcs.map( ( arc, index ) => { + const [ centroidX, centroidY ] = pie.path.centroid( arc ); + const hasSpaceForLabel = + arc.endAngle - arc.startAngle >= 0.25; + const handleMouseMove = ( + event: MouseEvent< SVGElement > + ) => { + if ( ! withTooltips ) { + return; + } + + // Don't show tooltip until container bounds are measured + if ( + containerBounds.width === 0 || + containerBounds.height === 0 + ) { + return; + } + + // Use clientX/Y and subtract containerBounds to cancel out any stale offset. + // TooltipInPortal calculates: tooltipLeft + containerBounds.left + scrollX + // By passing (clientX - containerBounds.left), we get: + // (clientX - containerBounds.left) + containerBounds.left + scrollX = clientX + scrollX + // This gives correct page coordinates regardless of stale bounds. + showTooltip( { + tooltipData: arc.data, + tooltipLeft: + event.clientX - + containerBounds.left + + tooltipOffsetX, + tooltipTop: + event.clientY - + containerBounds.top + + tooltipOffsetY, + } ); + }; + + const pathProps: SVGProps< SVGPathElement > & { + 'data-testid'?: string; + } = { + d: pie.path( arc ) || '', + fill: accessors.fill( arc.data ), + 'data-testid': 'pie-segment', + }; + + const groupProps: SVGProps< SVGGElement > = {}; + if ( withTooltips ) { + groupProps.onMouseMove = handleMouseMove; + groupProps.onMouseLeave = onMouseLeave; + } + + // Estimate text width more accurately for background sizing + const fontSize = 12; + const estimatedTextWidth = getStringWidth( + arc.data.label, + { fontSize } + ); + const labelPadding = 6; + const backgroundWidth = + estimatedTextWidth + labelPadding * 2; + const backgroundHeight = + fontSize + labelPadding * 2; + + return ( + + + { showLabels && hasSpaceForLabel && ( + + { providerTheme.labelBackgroundColor && ( + + ) } + + { arc.data.label } + + ) } - - { arc.data.label } - - ) } - - ); - } ); - } } - - ) } - - { /* Render SVG children (like Group, Text) inside the SVG */ } - { ! allSegmentsHidden && svgChildren } - - -
+ ); + } ); + } } + + ) } + + { /* Render SVG children (like Group, Text) inside the SVG */ } + { ! allSegmentsHidden && svgChildren } + + +
+ + ); + } } - ); }; From 6c4915e5bbb09d367b37cbbab2d22970eb3a4535 Mon Sep 17 00:00:00 2001 From: Adam Wood <1017872+adamwoodnz@users.noreply.github.com> Date: Thu, 12 Mar 2026 15:16:10 +1300 Subject: [PATCH 10/37] Charts: Add render prop and measurement support to ChartLayout ChartLayout now accepts children as either ReactNode or a render prop receiving ContentMeasurements (contentRef, contentWidth, contentHeight, isMeasured). This allows charts to consolidate their useElementSize calls into the shared layout component. Adds waitForMeasurement prop that hides the layout until content is measured. The legacy isWaitingForMeasurement prop is preserved as deprecated for backward compatibility. Co-Authored-By: Claude Opus 4.6 --- .../components/chart-layout/chart-layout.tsx | 50 ++++++++++++++++--- .../src/components/chart-layout/index.ts | 2 +- 2 files changed, 43 insertions(+), 9 deletions(-) diff --git a/projects/js-packages/charts/src/components/chart-layout/chart-layout.tsx b/projects/js-packages/charts/src/components/chart-layout/chart-layout.tsx index a0da71fa6f40..9171c53ca534 100644 --- a/projects/js-packages/charts/src/components/chart-layout/chart-layout.tsx +++ b/projects/js-packages/charts/src/components/chart-layout/chart-layout.tsx @@ -2,12 +2,27 @@ import { Stack } from '@wordpress/ui'; import clsx from 'clsx'; import { forwardRef } from 'react'; import { renderLegendSlot } from '../../charts/private/chart-composition'; +import { useElementSize } from '../../hooks'; import styles from './chart-layout.module.scss'; import type { LegendChild } from '../../charts/private/chart-composition/use-chart-children'; import type { LegendPosition } from '../../types'; import type { GapSize } from '@wordpress/theme'; import type { CSSProperties, ReactNode } from 'react'; +/** + * Measurements provided to the render prop when ChartLayout handles resize listening. + */ +export interface ContentMeasurements { + /** Ref callback to attach to the content element being measured */ + contentRef: ( node: HTMLDivElement | null ) => void; + /** Measured width of the content area in pixels */ + contentWidth: number; + /** Measured height of the content area in pixels */ + contentHeight: number; + /** True once the content area has been measured. Always true when waitForMeasurement is not set. */ + isMeasured: boolean; +} + export interface ChartLayoutProps { /** Position for the prop-based legend element */ legendPosition: LegendPosition; @@ -15,11 +30,13 @@ export interface ChartLayoutProps { legendElement?: ReactNode; /** Legend children from the composition API */ legendChildren: LegendChild[]; - /** Chart content rendered between legend slots */ - children: ReactNode; + /** Chart content — either a ReactNode or a render prop receiving content measurements */ + children: ReactNode | ( ( measurements: ContentMeasurements ) => ReactNode ); /** Content rendered after the bottom legend (e.g., nonLegendChildren, htmlChildren, tooltips) */ trailingContent?: ReactNode; - /** When true, sets visibility: hidden on the container. Used by charts that need measurement before rendering. */ + /** When true, hides the layout until content measurement is available */ + waitForMeasurement?: boolean; + /** @deprecated Use waitForMeasurement instead */ isWaitingForMeasurement?: boolean; /** Gap between Stack items */ gap?: GapSize; @@ -41,6 +58,7 @@ export const ChartLayout = forwardRef< HTMLDivElement, ChartLayoutProps >( legendChildren, children, trailingContent, + waitForMeasurement, isWaitingForMeasurement, gap, className, @@ -50,10 +68,26 @@ export const ChartLayout = forwardRef< HTMLDivElement, ChartLayoutProps >( }, ref ) => { - const visibilityStyle = - isWaitingForMeasurement !== undefined - ? { visibility: ( isWaitingForMeasurement ? 'hidden' : 'visible' ) as const } - : {}; + const [ contentRef, contentWidth, contentHeight ] = useElementSize< HTMLDivElement >(); + const isMeasured = contentHeight > 0; + + // Determine visibility: new waitForMeasurement prop takes precedence over legacy isWaitingForMeasurement + let visibilityStyle: { visibility?: 'hidden' | 'visible' } = {}; + if ( waitForMeasurement !== undefined ) { + // New API: hide until measured + const isHidden = waitForMeasurement && ! isMeasured; + visibilityStyle = { visibility: isHidden ? 'hidden' : 'visible' }; + } else if ( isWaitingForMeasurement !== undefined ) { + // Legacy API + visibilityStyle = { + visibility: isWaitingForMeasurement ? 'hidden' : 'visible', + }; + } + + const renderedChildren = + typeof children === 'function' + ? children( { contentRef, contentWidth, contentHeight, isMeasured } ) + : children; return ( ( { legendPosition === 'top' && legendElement } { renderLegendSlot( legendChildren, 'top' ) } - { children } + { renderedChildren } { legendPosition === 'bottom' && legendElement } { renderLegendSlot( legendChildren, 'bottom' ) } diff --git a/projects/js-packages/charts/src/components/chart-layout/index.ts b/projects/js-packages/charts/src/components/chart-layout/index.ts index 8e997d8803af..7e4ff79c6c63 100644 --- a/projects/js-packages/charts/src/components/chart-layout/index.ts +++ b/projects/js-packages/charts/src/components/chart-layout/index.ts @@ -1,2 +1,2 @@ export { ChartLayout } from './chart-layout'; -export type { ChartLayoutProps } from './chart-layout'; +export type { ChartLayoutProps, ContentMeasurements } from './chart-layout'; From 85f1a86c6a67c975c79133aa872b6880c99f57cd Mon Sep 17 00:00:00 2001 From: Adam Wood <1017872+adamwoodnz@users.noreply.github.com> Date: Thu, 12 Mar 2026 15:18:35 +1300 Subject: [PATCH 11/37] Charts: Fix PieChart SingleChartContext.Provider placement Move SingleChartContext.Provider to wrap ChartLayout (outside) so composition API legends in legend slots can access the chart context. Keep inner provider inside the render prop for measured dimensions. Remove waitForMeasurement since PieChart uses fallback dimensions (300x300) when unmeasured and doesn't need to hide. Co-Authored-By: Claude Opus 4.6 --- .../charts/src/charts/pie-chart/pie-chart.tsx | 286 ++++++++---------- 1 file changed, 130 insertions(+), 156 deletions(-) diff --git a/projects/js-packages/charts/src/charts/pie-chart/pie-chart.tsx b/projects/js-packages/charts/src/charts/pie-chart/pie-chart.tsx index 95051e1808a3..fc07816f1abd 100644 --- a/projects/js-packages/charts/src/charts/pie-chart/pie-chart.tsx +++ b/projects/js-packages/charts/src/charts/pie-chart/pie-chart.tsx @@ -294,8 +294,9 @@ const PieChartInternal = ( { ); return ( - +
- - - - - - { allSegmentsHidden ? ( - - { __( - 'All segments are hidden. Click legend items to show data.', - 'jetpack-charts' - ) } - - ) : ( - - data={ dataWithIndex } - pieValue={ accessors.value } - outerRadius={ outerRadius } + + - { pie => { - return pie.arcs.map( ( arc, index ) => { - const [ centroidX, centroidY ] = pie.path.centroid( arc ); - const hasSpaceForLabel = - arc.endAngle - arc.startAngle >= 0.25; - const handleMouseMove = ( - event: MouseEvent< SVGElement > - ) => { - if ( ! withTooltips ) { - return; + /> + + + + { allSegmentsHidden ? ( + + { __( + 'All segments are hidden. Click legend items to show data.', + 'jetpack-charts' + ) } + + ) : ( + + data={ dataWithIndex } + pieValue={ accessors.value } + outerRadius={ outerRadius } + innerRadius={ innerRadius } + padAngle={ padAngle } + cornerRadius={ cornerRadius } + > + { pie => { + return pie.arcs.map( ( arc, index ) => { + const [ centroidX, centroidY ] = pie.path.centroid( arc ); + const hasSpaceForLabel = arc.endAngle - arc.startAngle >= 0.25; + const handleMouseMove = ( event: MouseEvent< SVGElement > ) => { + if ( ! withTooltips ) { + return; + } + + // Don't show tooltip until container bounds are measured + if ( containerBounds.width === 0 || containerBounds.height === 0 ) { + return; + } + + // Use clientX/Y and subtract containerBounds to cancel out any stale offset. + // TooltipInPortal calculates: tooltipLeft + containerBounds.left + scrollX + // By passing (clientX - containerBounds.left), we get: + // (clientX - containerBounds.left) + containerBounds.left + scrollX = clientX + scrollX + // This gives correct page coordinates regardless of stale bounds. + showTooltip( { + tooltipData: arc.data, + tooltipLeft: event.clientX - containerBounds.left + tooltipOffsetX, + tooltipTop: event.clientY - containerBounds.top + tooltipOffsetY, + } ); + }; + + const pathProps: SVGProps< SVGPathElement > & { + 'data-testid'?: string; + } = { + d: pie.path( arc ) || '', + fill: accessors.fill( arc.data ), + 'data-testid': 'pie-segment', + }; + + const groupProps: SVGProps< SVGGElement > = {}; + if ( withTooltips ) { + groupProps.onMouseMove = handleMouseMove; + groupProps.onMouseLeave = onMouseLeave; } - // Don't show tooltip until container bounds are measured - if ( - containerBounds.width === 0 || - containerBounds.height === 0 - ) { - return; - } - - // Use clientX/Y and subtract containerBounds to cancel out any stale offset. - // TooltipInPortal calculates: tooltipLeft + containerBounds.left + scrollX - // By passing (clientX - containerBounds.left), we get: - // (clientX - containerBounds.left) + containerBounds.left + scrollX = clientX + scrollX - // This gives correct page coordinates regardless of stale bounds. - showTooltip( { - tooltipData: arc.data, - tooltipLeft: - event.clientX - - containerBounds.left + - tooltipOffsetX, - tooltipTop: - event.clientY - - containerBounds.top + - tooltipOffsetY, + // Estimate text width more accurately for background sizing + const fontSize = 12; + const estimatedTextWidth = getStringWidth( arc.data.label, { + fontSize, } ); - }; - - const pathProps: SVGProps< SVGPathElement > & { - 'data-testid'?: string; - } = { - d: pie.path( arc ) || '', - fill: accessors.fill( arc.data ), - 'data-testid': 'pie-segment', - }; - - const groupProps: SVGProps< SVGGElement > = {}; - if ( withTooltips ) { - groupProps.onMouseMove = handleMouseMove; - groupProps.onMouseLeave = onMouseLeave; - } - - // Estimate text width more accurately for background sizing - const fontSize = 12; - const estimatedTextWidth = getStringWidth( - arc.data.label, - { fontSize } - ); - const labelPadding = 6; - const backgroundWidth = - estimatedTextWidth + labelPadding * 2; - const backgroundHeight = - fontSize + labelPadding * 2; - - return ( - - - { showLabels && hasSpaceForLabel && ( - - { providerTheme.labelBackgroundColor && ( - + + { showLabels && hasSpaceForLabel && ( + + { providerTheme.labelBackgroundColor && ( + + ) } + - ) } - - { arc.data.label } - - - ) } - - ); - } ); - } } - - ) } - - { /* Render SVG children (like Group, Text) inside the SVG */ } - { ! allSegmentsHidden && svgChildren } - - + > + { arc.data.label } + + + ) } + + ); + } ); + } } + + ) } + + { /* Render SVG children (like Group, Text) inside the SVG */ } + { ! allSegmentsHidden && svgChildren } + +
); } } + ); }; From 6b97bbb50b8309e34ca28cafcda2987f0b1a129f Mon Sep 17 00:00:00 2001 From: Adam Wood <1017872+adamwoodnz@users.noreply.github.com> Date: Thu, 12 Mar 2026 15:23:48 +1300 Subject: [PATCH 12/37] Charts: Migrate LineChart, BarChart, PieSemiCircleChart to ChartLayout render prop Remove individual useElementSize calls from each chart and use ChartLayout's render prop to receive content measurements instead. Pattern: outer SingleChartContext.Provider with fallback dimensions (for composition API legends in layout slots), inner provider inside render prop with measured dimensions (for chart internals). Co-Authored-By: Claude Opus 4.6 --- .../charts/src/charts/bar-chart/bar-chart.tsx | 232 ++++++------- .../src/charts/line-chart/line-chart.tsx | 326 +++++++++--------- .../charts/src/charts/pie-chart/pie-chart.tsx | 266 +++++++------- .../pie-semi-circle-chart.tsx | 224 ++++++------ 4 files changed, 520 insertions(+), 528 deletions(-) diff --git a/projects/js-packages/charts/src/charts/bar-chart/bar-chart.tsx b/projects/js-packages/charts/src/charts/bar-chart/bar-chart.tsx index 39ac9805ae15..7c8929f99bf7 100644 --- a/projects/js-packages/charts/src/charts/bar-chart/bar-chart.tsx +++ b/projects/js-packages/charts/src/charts/bar-chart/bar-chart.tsx @@ -12,7 +12,6 @@ import { useChartDataTransform, useZeroValueDisplay, useChartMargin, - useElementSize, usePrefersReducedMotion, } from '../../hooks'; import { @@ -113,19 +112,12 @@ const BarChartInternal: FC< BarChartProps > = ( { const legendItems = useChartLegendItems( dataSorted ); const chartOptions = useBarChartOptions( dataWithVisibleZeros, horizontal, options ); const defaultMargin = useChartMargin( height, chartOptions, dataSorted, theme, horizontal ); - const [ svgWrapperRef, , svgWrapperHeight ] = useElementSize< HTMLDivElement >(); const chartRef = useRef< HTMLDivElement >( null ); // Process children for composition API (Legend, etc.) const { legendChildren, nonLegendChildren } = useChartChildren( children, 'BarChart' ); const hasLegendChild = legendChildren.length > 0; - - // Use the measured SVG wrapper height, falling back to the passed height if provided. - // When there's a legend (via prop or composition), we must wait for measurement because - // the legend takes space and the svg-wrapper height will be less than the total height. - const chartHeight = svgWrapperHeight > 0 ? svgWrapperHeight : height; const hasLegend = showLegend || hasLegendChild; - const isWaitingForMeasurement = hasLegend ? svgWrapperHeight === 0 : ! chartHeight; const [ selectedIndex, setSelectedIndex ] = useState< number | undefined >( undefined ); const [ isNavigating, setIsNavigating ] = useState( false ); @@ -343,14 +335,13 @@ const BarChartInternal: FC< BarChartProps > = ( { value={ { chartId, chartWidth: width, - chartHeight, + chartHeight: height || 0, } } > = ( { data-chart-id={ `bar-chart-${ chartId }` } trailingContent={ nonLegendChildren } > -
- { ! isWaitingForMeasurement && ( -
- - - - { withPatterns && ( - <> - - { dataSorted.map( ( seriesData, index ) => - renderPattern( - index, - getElementStyles( { data: seriesData, index } ).color - ) - ) } - - - - ) } - - { highlightedBarStyle && } - - { allSeriesHidden ? ( - { + const chartHeight = contentHeight > 0 ? contentHeight : height; + const isWaitingForMeasurement = hasLegend ? contentHeight === 0 : ! chartHeight; + + return ( +
+ { ! isWaitingForMeasurement && ( +
+ - { __( - 'All series are hidden. Click legend items to show data.', - 'jetpack-charts' + + + { withPatterns && ( + <> + + { dataSorted.map( ( seriesData, index ) => + renderPattern( + index, + getElementStyles( { data: seriesData, index } ).color + ) + ) } + + + ) } - - ) : null } - - - { seriesWithVisibility.map( ( { series: seriesData, index, isVisible } ) => { - // Skip rendering invisible series - if ( ! isVisible ) { - return null; - } - - return ( - { highlightedBarStyle } } + + { allSeriesHidden ? ( + + { __( + 'All series are hidden. Click legend items to show data.', + 'jetpack-charts' + ) } + + ) : null } + + + { seriesWithVisibility.map( ( { series: seriesData, index, isVisible } ) => { + // Skip rendering invisible series + if ( ! isVisible ) { + return null; + } + + return ( + + ); + } ) } + + + + + + { withTooltips && ( + - ); - } ) } - - - - - - { withTooltips && ( - - ) } - + ) } + +
+ ) }
- ) } -
+ ); + } } ); diff --git a/projects/js-packages/charts/src/charts/line-chart/line-chart.tsx b/projects/js-packages/charts/src/charts/line-chart/line-chart.tsx index c9b949d03cdf..4af2fa8efd50 100644 --- a/projects/js-packages/charts/src/charts/line-chart/line-chart.tsx +++ b/projects/js-packages/charts/src/charts/line-chart/line-chart.tsx @@ -14,7 +14,6 @@ import { useXYChartTheme, useChartDataTransform, useChartMargin, - useElementSize, usePrefersReducedMotion, } from '../../hooks'; import { @@ -285,7 +284,6 @@ const LineChartInternal = forwardRef< SingleChartRef, LineChartProps >( const providerTheme = useGlobalChartsTheme(); const theme = useXYChartTheme( data ); const chartId = useChartId( providedChartId ); - const [ svgWrapperRef, , svgWrapperHeight ] = useElementSize< HTMLDivElement >(); const chartRef = useRef< HTMLDivElement >( null ); const [ selectedIndex, setSelectedIndex ] = useState< number | undefined >( undefined ); const [ isNavigating, setIsNavigating ] = useState( false ); @@ -295,12 +293,7 @@ const LineChartInternal = forwardRef< SingleChartRef, LineChartProps >( const { legendChildren, nonLegendChildren } = useChartChildren( children, 'LineChart' ); const hasLegendChild = legendChildren.length > 0; - // Use the measured SVG wrapper height, falling back to the passed height if provided. - // When there's a legend (via prop or composition), we must wait for measurement because - // the legend takes space and the svg-wrapper height will be less than the total height. - const chartHeight = svgWrapperHeight > 0 ? svgWrapperHeight : height; const hasLegend = showLegend || hasLegendChild; - const isWaitingForMeasurement = hasLegend ? svgWrapperHeight === 0 : ! chartHeight; // Forward the external ref to the internal ref useImperativeHandle( @@ -472,14 +465,13 @@ const LineChartInternal = forwardRef< SingleChartRef, LineChartProps >( chartId, chartRef: internalChartRef, chartWidth: width, - chartHeight, + chartHeight: height || 0, } } > ( data-testid="line-chart" trailingContent={ nonLegendChildren } > -
- { ! isWaitingForMeasurement && ( -
- - { gridVisibility !== 'none' && } - { chartOptions.axis.x.display && } - { chartOptions.axis.y.display && } - - { allSeriesHidden ? ( - { + // Use the measured height, falling back to the passed height if provided. + const chartHeight = contentHeight > 0 ? contentHeight : height; + const isWaitingForMeasurement = hasLegend ? contentHeight === 0 : ! chartHeight; + + return ( +
+ { ! isWaitingForMeasurement && ( +
+ - { __( - 'All series are hidden. Click legend items to show data.', - 'jetpack-charts' - ) } - - ) : null } - - { seriesWithVisibility.map( ( { series: seriesData, index, isVisible } ) => { - // Skip rendering invisible series - if ( ! isVisible ) { - return null; - } - - const { color, lineStyles, glyph } = getElementStyles( { - data: seriesData, - index, - } ); - - const lineProps = { - stroke: color, - ...lineStyles, - }; - - return ( - - { withGradientFill && ( - - { seriesData.options?.gradient?.stops?.map( ( stop, stopIndex ) => ( - } + { chartOptions.axis.x.display && } + { chartOptions.axis.y.display && } + + { allSeriesHidden ? ( + + { __( + 'All series are hidden. Click legend items to show data.', + 'jetpack-charts' + ) } + + ) : null } + + { seriesWithVisibility.map( ( { series: seriesData, index, isVisible } ) => { + // Skip rendering invisible series + if ( ! isVisible ) { + return null; + } + + const { color, lineStyles, glyph } = getElementStyles( { + data: seriesData, + index, + } ); + + const lineProps = { + stroke: color, + ...lineStyles, + }; + + return ( + + { withGradientFill && ( + + { seriesData.options?.gradient?.stops?.map( ( stop, stopIndex ) => ( + + ) ) } + + ) } + + + { withStartGlyphs && ( + + ) } + + { withEndGlyphs && ( + - ) ) } - - ) } - + ); + } ) } + + { withTooltips && ( + + ) } - { withStartGlyphs && ( - - ) } - - { withEndGlyphs && ( - - ) } - - ); - } ) } - - { withTooltips && ( - - ) } - - { /* Component to expose scale data via ref */ } - - + { /* Component to expose scale data via ref */ } + + +
+ ) }
- ) } -
+ ); + } } ); diff --git a/projects/js-packages/charts/src/charts/pie-chart/pie-chart.tsx b/projects/js-packages/charts/src/charts/pie-chart/pie-chart.tsx index fc07816f1abd..fd39ff6de6aa 100644 --- a/projects/js-packages/charts/src/charts/pie-chart/pie-chart.tsx +++ b/projects/js-packages/charts/src/charts/pie-chart/pie-chart.tsx @@ -346,149 +346,141 @@ const PieChartInternal = ( { : 0; return ( - -
- + + + + + + - - + { __( + 'All segments are hidden. Click legend items to show data.', + 'jetpack-charts' + ) } + + ) : ( + + data={ dataWithIndex } + pieValue={ accessors.value } + outerRadius={ outerRadius } innerRadius={ innerRadius } - /> - - - - { allSegmentsHidden ? ( - - { __( - 'All segments are hidden. Click legend items to show data.', - 'jetpack-charts' - ) } - - ) : ( - - data={ dataWithIndex } - pieValue={ accessors.value } - outerRadius={ outerRadius } - innerRadius={ innerRadius } - padAngle={ padAngle } - cornerRadius={ cornerRadius } - > - { pie => { - return pie.arcs.map( ( arc, index ) => { - const [ centroidX, centroidY ] = pie.path.centroid( arc ); - const hasSpaceForLabel = arc.endAngle - arc.startAngle >= 0.25; - const handleMouseMove = ( event: MouseEvent< SVGElement > ) => { - if ( ! withTooltips ) { - return; - } - - // Don't show tooltip until container bounds are measured - if ( containerBounds.width === 0 || containerBounds.height === 0 ) { - return; - } - - // Use clientX/Y and subtract containerBounds to cancel out any stale offset. - // TooltipInPortal calculates: tooltipLeft + containerBounds.left + scrollX - // By passing (clientX - containerBounds.left), we get: - // (clientX - containerBounds.left) + containerBounds.left + scrollX = clientX + scrollX - // This gives correct page coordinates regardless of stale bounds. - showTooltip( { - tooltipData: arc.data, - tooltipLeft: event.clientX - containerBounds.left + tooltipOffsetX, - tooltipTop: event.clientY - containerBounds.top + tooltipOffsetY, - } ); - }; - - const pathProps: SVGProps< SVGPathElement > & { - 'data-testid'?: string; - } = { - d: pie.path( arc ) || '', - fill: accessors.fill( arc.data ), - 'data-testid': 'pie-segment', - }; - - const groupProps: SVGProps< SVGGElement > = {}; - if ( withTooltips ) { - groupProps.onMouseMove = handleMouseMove; - groupProps.onMouseLeave = onMouseLeave; + padAngle={ padAngle } + cornerRadius={ cornerRadius } + > + { pie => { + return pie.arcs.map( ( arc, index ) => { + const [ centroidX, centroidY ] = pie.path.centroid( arc ); + const hasSpaceForLabel = arc.endAngle - arc.startAngle >= 0.25; + const handleMouseMove = ( event: MouseEvent< SVGElement > ) => { + if ( ! withTooltips ) { + return; } - // Estimate text width more accurately for background sizing - const fontSize = 12; - const estimatedTextWidth = getStringWidth( arc.data.label, { - fontSize, + // Don't show tooltip until container bounds are measured + if ( containerBounds.width === 0 || containerBounds.height === 0 ) { + return; + } + + // Use clientX/Y and subtract containerBounds to cancel out any stale offset. + // TooltipInPortal calculates: tooltipLeft + containerBounds.left + scrollX + // By passing (clientX - containerBounds.left), we get: + // (clientX - containerBounds.left) + containerBounds.left + scrollX = clientX + scrollX + // This gives correct page coordinates regardless of stale bounds. + showTooltip( { + tooltipData: arc.data, + tooltipLeft: event.clientX - containerBounds.left + tooltipOffsetX, + tooltipTop: event.clientY - containerBounds.top + tooltipOffsetY, } ); - const labelPadding = 6; - const backgroundWidth = estimatedTextWidth + labelPadding * 2; - const backgroundHeight = fontSize + labelPadding * 2; - - return ( - - - { showLabels && hasSpaceForLabel && ( - - { providerTheme.labelBackgroundColor && ( - - ) } - - { arc.data.label } - - - ) } - - ); + }; + + const pathProps: SVGProps< SVGPathElement > & { + 'data-testid'?: string; + } = { + d: pie.path( arc ) || '', + fill: accessors.fill( arc.data ), + 'data-testid': 'pie-segment', + }; + + const groupProps: SVGProps< SVGGElement > = {}; + if ( withTooltips ) { + groupProps.onMouseMove = handleMouseMove; + groupProps.onMouseLeave = onMouseLeave; + } + + // Estimate text width more accurately for background sizing + const fontSize = 12; + const estimatedTextWidth = getStringWidth( arc.data.label, { + fontSize, } ); - } } - - ) } - - { /* Render SVG children (like Group, Text) inside the SVG */ } - { ! allSegmentsHidden && svgChildren } - - -
- + const labelPadding = 6; + const backgroundWidth = estimatedTextWidth + labelPadding * 2; + const backgroundHeight = fontSize + labelPadding * 2; + + return ( + + + { showLabels && hasSpaceForLabel && ( + + { providerTheme.labelBackgroundColor && ( + + ) } + + { arc.data.label } + + + ) } + + ); + } ); + } } + + ) } + + { /* Render SVG children (like Group, Text) inside the SVG */ } + { ! allSegmentsHidden && svgChildren } + + +
); } } diff --git a/projects/js-packages/charts/src/charts/pie-semi-circle-chart/pie-semi-circle-chart.tsx b/projects/js-packages/charts/src/charts/pie-semi-circle-chart/pie-semi-circle-chart.tsx index 3583b5554dc7..d3b0b53fb71a 100644 --- a/projects/js-packages/charts/src/charts/pie-semi-circle-chart/pie-semi-circle-chart.tsx +++ b/projects/js-packages/charts/src/charts/pie-semi-circle-chart/pie-semi-circle-chart.tsx @@ -8,7 +8,7 @@ import { useCallback, useContext, useMemo } from 'react'; import { ChartLayout } from '../../components/chart-layout'; import { Legend, useChartLegendItems } from '../../components/legend'; import { BaseTooltip } from '../../components/tooltip'; -import { useElementSize, useInteractiveLegendData, usePrefersReducedMotion } from '../../hooks'; +import { useInteractiveLegendData, usePrefersReducedMotion } from '../../hooks'; import { GlobalChartsProvider, useChartId, @@ -176,8 +176,6 @@ const PieSemiCircleChartInternal: FC< PieSemiCircleChartProps > = ( { const legendPosition = legend.position ?? 'bottom'; const chartId = useChartId( providedChartId ); - // Measure the SVG wrapper to calculate constrained dimensions - const [ svgWrapperRef, svgWrapperWidth, svgWrapperHeight ] = useElementSize< HTMLDivElement >(); const { tooltipOpen, tooltipLeft, tooltipTop, tooltipData, hideTooltip, showTooltip } = useTooltip< DataPointPercentage >(); @@ -310,18 +308,6 @@ const PieSemiCircleChartInternal: FC< PieSemiCircleChartProps > = ( { ); } - // Calculate chart dimensions maintaining the 2:1 width-to-height ratio. - // Use measured SVG wrapper dimensions to respect height constraints, falling back - // to explicit props during initial render before measurement is available. - const availableWidth = svgWrapperWidth > 0 ? svgWrapperWidth : effectiveWidth; - const availableHeight = - svgWrapperHeight > 0 ? svgWrapperHeight : propHeight || effectiveWidth / 2; - // Constrain width so that height (= width / 2) never exceeds the available height - const width = Math.min( availableWidth, availableHeight * 2 ); - const height = width / 2; - const radius = height; // For a semi-circle, radius equals the SVG height - const innerRadius = radius * ( 1 - thickness ); - // Map data with index for color assignment // When interactive, we need to find the original index to maintain consistent colors const dataWithIndex = visibleData.map( d => { @@ -352,13 +338,7 @@ const PieSemiCircleChartInternal: FC< PieSemiCircleChartProps > = ( { ); return ( - + = ( { } > -
- - - - - - { /* Main chart group centered horizontally and positioned at bottom */ } - - { allSegmentsHidden ? ( - - { __( - 'All segments are hidden. Click legend items to show data.', - 'jetpack-charts' - ) } - - ) : ( - <> - { /* Pie chart */ } - - data={ dataWithIndex } - pieValue={ accessors.value } - outerRadius={ radius } + { ( { contentRef: measureRef, contentWidth, contentHeight } ) => { + // Calculate chart dimensions maintaining the 2:1 width-to-height ratio. + // Use measured dimensions to respect height constraints, falling back + // to explicit props during initial render before measurement is available. + const availableWidth = contentWidth > 0 ? contentWidth : effectiveWidth; + const availableHeight = + contentHeight > 0 ? contentHeight : propHeight || effectiveWidth / 2; + // Constrain width so that height (= width / 2) never exceeds the available height + const width = Math.min( availableWidth, availableHeight * 2 ); + const height = width / 2; + const radius = height; // For a semi-circle, radius equals the SVG height + const innerRadius = radius * ( 1 - thickness ); + + return ( +
+ + + - { pie => { - return pie.arcs.map( arc => ( - - - - ) ); - } } - - - { /* Label and note text */ } - - - { label } - - + + + { /* Main chart group centered horizontally and positioned at bottom */ } + + { allSegmentsHidden ? ( + - { note } - - - - { /* Render SVG children from composition API */ } - { ! allSegmentsHidden && svgChildren } - - ) } - - -
+ { __( + 'All segments are hidden. Click legend items to show data.', + 'jetpack-charts' + ) } + + ) : ( + <> + { /* Pie chart */ } + + data={ dataWithIndex } + pieValue={ accessors.value } + outerRadius={ radius } + innerRadius={ innerRadius } + cornerRadius={ 3 } + padAngle={ PAD_ANGLE } + startAngle={ startAngle } + endAngle={ endAngle } + pieSort={ accessors.sort } + > + { pie => { + return pie.arcs.map( arc => ( + + + + ) ); + } } + + + { /* Label and note text */ } + + + { label } + + + { note } + + + + { /* Render SVG children from composition API */ } + { ! allSegmentsHidden && svgChildren } + + ) } + + +
+ ); + } } ); From 216809ccaf534e6435d841fe1ef1a594bc1a25d5 Mon Sep 17 00:00:00 2001 From: Changelog Bot Date: Thu, 12 Mar 2026 02:33:52 +0000 Subject: [PATCH 13/37] Add changelog entries. --- projects/js-packages/charts/changelog/pr-47554 | 4 ++++ 1 file changed, 4 insertions(+) create mode 100644 projects/js-packages/charts/changelog/pr-47554 diff --git a/projects/js-packages/charts/changelog/pr-47554 b/projects/js-packages/charts/changelog/pr-47554 new file mode 100644 index 000000000000..511329c3f103 --- /dev/null +++ b/projects/js-packages/charts/changelog/pr-47554 @@ -0,0 +1,4 @@ +Significance: minor +Type: added + +ChartLayout: Add component for shared chart and legend layout. From 2956ebb86d0834c7db122da6564acd9fc824a034 Mon Sep 17 00:00:00 2001 From: Adam Wood <1017872+adamwoodnz@users.noreply.github.com> Date: Thu, 12 Mar 2026 15:57:07 +1300 Subject: [PATCH 14/37] test(charts): Simplify ChartLayout DOM order assertions Replace the expectDomOrder helper with direct compareDocumentPosition assertions. The helper required eslint-disable no-bitwise for the masking check; inline expect(...).toBe(Node.DOCUMENT_POSITION_FOLLOWING) is clearer and avoids the suppression. Made-with: Cursor --- .../chart-layout/test/chart-layout.test.tsx | 27 +++++++------------ 1 file changed, 9 insertions(+), 18 deletions(-) diff --git a/projects/js-packages/charts/src/components/chart-layout/test/chart-layout.test.tsx b/projects/js-packages/charts/src/components/chart-layout/test/chart-layout.test.tsx index 4d53e9343703..11fbd8ac371a 100644 --- a/projects/js-packages/charts/src/components/chart-layout/test/chart-layout.test.tsx +++ b/projects/js-packages/charts/src/components/chart-layout/test/chart-layout.test.tsx @@ -1,4 +1,3 @@ -/* eslint-disable no-bitwise, jest/expect-expect */ import { render, screen } from '@testing-library/react'; import { renderLegendSlot } from '../../../charts/private/chart-composition'; import { ChartLayout } from '../chart-layout'; @@ -11,18 +10,6 @@ jest.mock( '../../../charts/private/chart-composition', () => ( { const mockRenderLegendSlot = renderLegendSlot as jest.Mock; -/** - * Asserts that `earlier` appears before `later` in DOM order. - * - * @param {HTMLElement} earlier - Element expected to appear first - * @param {HTMLElement} later - Element expected to appear second - */ -function expectDomOrder( earlier: HTMLElement, later: HTMLElement ) { - expect( - earlier.compareDocumentPosition( later ) & Node.DOCUMENT_POSITION_FOLLOWING - ).toBeTruthy(); -} - describe( 'ChartLayout', () => { beforeEach( () => { mockRenderLegendSlot.mockReturnValue( [] ); @@ -44,8 +31,9 @@ describe( 'ChartLayout', () => {
Chart
); - // Legend should come before content in DOM order - expectDomOrder( screen.getByTestId( 'legend' ), screen.getByTestId( 'chart-content' ) ); + const legend = screen.getByTestId( 'legend' ); + const content = screen.getByTestId( 'chart-content' ); + expect( legend.compareDocumentPosition( content ) ).toBe( Node.DOCUMENT_POSITION_FOLLOWING ); } ); it( 'renders legend element at bottom when legendPosition is bottom', () => { @@ -55,8 +43,9 @@ describe( 'ChartLayout', () => {
Chart
); - // Content should come before legend in DOM order - expectDomOrder( screen.getByTestId( 'chart-content' ), screen.getByTestId( 'legend' ) ); + const content = screen.getByTestId( 'chart-content' ); + const legend = screen.getByTestId( 'legend' ); + expect( content.compareDocumentPosition( legend ) ).toBe( Node.DOCUMENT_POSITION_FOLLOWING ); } ); it( 'does not render legend element when it is false/null', () => { @@ -166,6 +155,8 @@ describe( 'ChartLayout', () => {
Chart
); - expectDomOrder( screen.getByTestId( 'legend' ), screen.getByTestId( 'trailing' ) ); + const legend = screen.getByTestId( 'legend' ); + const trailing = screen.getByTestId( 'trailing' ); + expect( legend.compareDocumentPosition( trailing ) ).toBe( Node.DOCUMENT_POSITION_FOLLOWING ); } ); } ); From ee9b6f3b309817124e17ebdc67be5c126cf1b8d6 Mon Sep 17 00:00:00 2001 From: Adam Wood <1017872+adamwoodnz@users.noreply.github.com> Date: Thu, 12 Mar 2026 16:08:00 +1300 Subject: [PATCH 15/37] Charts: Extract shared CSS into ChartLayout, use Stack for pie centering MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ChartLayout now wraps render prop children in a content div with shared flex styles (flex: 1; min-height: 0; min-width: 0), removing duplicated svg-wrapper rules from line-chart, bar-chart, pie-chart, and pie-semi-circle-chart SCSS modules. Charts no longer need to attach contentRef — ChartLayout handles measurement internally via the content wrapper div. Replace custom flex centering CSS in pie-chart and pie-semi-circle-chart svg-wrappers with Stack components using align/justify props, per project guidelines preferring layout primitives over ad-hoc flex rules. Co-Authored-By: Claude Opus 4.6 --- .../plans/2026-03-12-chart-layout.md | 825 ++++++++++++++++++ .../charts/bar-chart/bar-chart.module.scss | 5 - .../charts/src/charts/bar-chart/bar-chart.tsx | 4 +- .../charts/line-chart/line-chart.module.scss | 5 - .../src/charts/line-chart/line-chart.tsx | 4 +- .../charts/pie-chart/pie-chart.module.scss | 10 - .../charts/src/charts/pie-chart/pie-chart.tsx | 7 +- .../pie-semi-circle-chart.module.scss | 11 - .../pie-semi-circle-chart.tsx | 7 +- .../chart-layout/chart-layout.module.scss | 10 +- .../components/chart-layout/chart-layout.tsx | 21 +- 11 files changed, 853 insertions(+), 56 deletions(-) create mode 100644 projects/js-packages/charts/docs/superpowers/plans/2026-03-12-chart-layout.md diff --git a/projects/js-packages/charts/docs/superpowers/plans/2026-03-12-chart-layout.md b/projects/js-packages/charts/docs/superpowers/plans/2026-03-12-chart-layout.md new file mode 100644 index 000000000000..3be4884905c2 --- /dev/null +++ b/projects/js-packages/charts/docs/superpowers/plans/2026-03-12-chart-layout.md @@ -0,0 +1,825 @@ +# ChartLayout Component Implementation Plan + +> **For agentic workers:** REQUIRED: Use superpowers:subagent-driven-development (if subagents available) or superpowers:executing-plans to implement this plan. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** Extract a shared `ChartLayout` component that encapsulates the duplicated Stack + legend positioning pattern across all 5 chart components. + +**Architecture:** ChartLayout is a thin layout wrapper that renders a `Stack direction="column"` with legend slots (top/bottom) sandwiching chart content passed as children. Each chart still owns its own measurement logic, chart rendering, legend element creation, and a11y handlers. ChartLayout handles only the structural layout concern. + +**Tech Stack:** React, TypeScript, SCSS modules, existing Stack/Legend components + +**Linear:** CHARTS-181 + +--- + +## File Structure + +| File | Responsibility | +|------|---------------| +| `src/components/chart-layout/chart-layout.tsx` | ChartLayout component | +| `src/components/chart-layout/chart-layout.module.scss` | Minimal layout styles (visibility hiding) | +| `src/components/chart-layout/index.ts` | Barrel export | +| `src/components/chart-layout/test/chart-layout.test.tsx` | Unit tests | +| `src/charts/line-chart/line-chart.tsx` | Refactor to use ChartLayout | +| `src/charts/bar-chart/bar-chart.tsx` | Refactor to use ChartLayout | +| `src/charts/pie-chart/pie-chart.tsx` | Refactor to use ChartLayout | +| `src/charts/pie-semi-circle-chart/pie-semi-circle-chart.tsx` | Refactor to use ChartLayout | +| `src/charts/leaderboard-chart/leaderboard-chart.tsx` | Refactor to use ChartLayout | + +All paths relative to `projects/js-packages/charts/`. + +--- + +## Chunk 1: ChartLayout Component + +### Task 1: Create ChartLayout test file + +**Files:** +- Create: `src/components/chart-layout/test/chart-layout.test.tsx` + +- [ ] **Step 1: Write failing tests for ChartLayout** + +```tsx +import { render, screen } from '@testing-library/react'; +import { ChartLayout } from '../chart-layout'; +import { renderLegendSlot } from '../../../charts/private/chart-composition'; +import type { LegendChild } from '../../../charts/private/chart-composition/use-chart-children'; + +// Mock renderLegendSlot since we test it separately +jest.mock( '../../../charts/private/chart-composition', () => ( { + renderLegendSlot: jest.fn( () => [] ), +} ) ); + +const mockRenderLegendSlot = renderLegendSlot as jest.Mock; + +describe( 'ChartLayout', () => { + beforeEach( () => { + mockRenderLegendSlot.mockReturnValue( [] ); + } ); + + it( 'renders children inside a column Stack', () => { + render( + +
Chart
+ + ); + expect( screen.getByTestId( 'chart-content' ) ).toBeInTheDocument(); + } ); + + it( 'renders legend element at top when legendPosition is top', () => { + const legendElement =
Legend
; + const { container } = render( + +
Chart
+ + ); + const stack = container.firstChild; + const legend = screen.getByTestId( 'legend' ); + const content = screen.getByTestId( 'chart-content' ); + // Legend should come before content in DOM order + expect( + legend.compareDocumentPosition( content ) & Node.DOCUMENT_POSITION_FOLLOWING + ).toBeTruthy(); + } ); + + it( 'renders legend element at bottom when legendPosition is bottom', () => { + const legendElement =
Legend
; + render( + +
Chart
+ + ); + const legend = screen.getByTestId( 'legend' ); + const content = screen.getByTestId( 'chart-content' ); + // Content should come before legend in DOM order + expect( + content.compareDocumentPosition( legend ) & Node.DOCUMENT_POSITION_FOLLOWING + ).toBeTruthy(); + } ); + + it( 'does not render legend element when it is false/null', () => { + render( + +
Chart
+ + ); + expect( screen.queryByTestId( 'legend' ) ).not.toBeInTheDocument(); + } ); + + it( 'calls renderLegendSlot for both positions', () => { + const legendChildren: LegendChild[] = []; + render( + +
Chart
+ + ); + expect( mockRenderLegendSlot ).toHaveBeenCalledWith( legendChildren, 'top' ); + expect( mockRenderLegendSlot ).toHaveBeenCalledWith( legendChildren, 'bottom' ); + } ); + + it( 'applies visibility hidden when isWaitingForMeasurement is true', () => { + render( + +
Chart
+ + ); + expect( screen.getByTestId( 'layout' ) ).toHaveStyle( { visibility: 'hidden' } ); + } ); + + it( 'applies visibility visible when isWaitingForMeasurement is false', () => { + render( + +
Chart
+ + ); + expect( screen.getByTestId( 'layout' ) ).toHaveStyle( { visibility: 'visible' } ); + } ); + + it( 'does not apply visibility style when isWaitingForMeasurement is undefined', () => { + render( + +
Chart
+ + ); + // No visibility in style when not opted in + expect( screen.getByTestId( 'layout' ).style.visibility ).toBe( '' ); + } ); + + it( 'passes className and style to Stack', () => { + render( + +
Chart
+ + ); + const layout = screen.getByTestId( 'layout' ); + expect( layout ).toHaveClass( 'my-chart' ); + expect( layout ).toHaveStyle( { width: '400px', height: '300px' } ); + } ); + + it( 'passes gap to Stack', () => { + const { container } = render( + +
Chart
+ + ); + // Stack renders gap as a CSS class or style — just verify it renders without error + expect( container.firstChild ).toBeInTheDocument(); + } ); + + it( 'forwards ref to Stack', () => { + const ref = jest.fn(); + render( + +
Chart
+ + ); + expect( ref ).toHaveBeenCalledWith( expect.any( HTMLElement ) ); + } ); + + it( 'renders trailing content after bottom legend', () => { + render( + Legend
} + legendChildren={ [] } + trailingContent={
Extra
} + > +
Chart
+ + ); + const legend = screen.getByTestId( 'legend' ); + const trailing = screen.getByTestId( 'trailing' ); + expect( + legend.compareDocumentPosition( trailing ) & Node.DOCUMENT_POSITION_FOLLOWING + ).toBeTruthy(); + } ); +} ); +``` + +- [ ] **Step 2: Run tests to verify they fail** + +Run: `cd projects/js-packages/charts && pnpm test -- --testPathPattern="chart-layout" --no-coverage` +Expected: FAIL — module not found + +- [ ] **Step 3: Commit test file** + +```bash +git add projects/js-packages/charts/src/components/chart-layout/test/chart-layout.test.tsx +git commit -m "Charts: Add ChartLayout component tests (red phase)" +``` + +--- + +### Task 2: Implement ChartLayout component + +**Files:** +- Create: `src/components/chart-layout/chart-layout.tsx` +- Create: `src/components/chart-layout/chart-layout.module.scss` +- Create: `src/components/chart-layout/index.ts` + +- [ ] **Step 4: Create the ChartLayout component** + +```tsx +// chart-layout.tsx +import clsx from 'clsx'; +import { forwardRef } from 'react'; +import { Stack } from '@wordpress/ui'; +import { renderLegendSlot } from '../../charts/private/chart-composition'; +import type { LegendChild } from '../../charts/private/chart-composition/use-chart-children'; +import type { LegendPosition } from '../../types'; +import type { CSSProperties, ReactNode } from 'react'; +import styles from './chart-layout.module.scss'; + +interface ChartLayoutProps { + /** Position for the prop-based legend element */ + legendPosition: LegendPosition; + /** The legend element rendered via the showLegend prop (false when hidden) */ + legendElement?: ReactNode; + /** Legend children from the composition API */ + legendChildren: LegendChild[]; + /** Chart content rendered between legend slots */ + children: ReactNode; + /** Content rendered after the bottom legend (e.g., nonLegendChildren, htmlChildren, tooltips) */ + trailingContent?: ReactNode; + /** When true, sets visibility: hidden on the container. Used by charts that need measurement before rendering. */ + isWaitingForMeasurement?: boolean; + /** Gap between Stack items */ + gap?: string; + /** Additional class names */ + className?: string; + /** Inline styles (width, height, etc.) */ + style?: CSSProperties; + /** Test ID for the container */ + 'data-testid'?: string; + /** Chart ID attribute */ + 'data-chart-id'?: string; +} + +export const ChartLayout = forwardRef< HTMLDivElement, ChartLayoutProps >( + ( + { + legendPosition, + legendElement, + legendChildren, + children, + trailingContent, + isWaitingForMeasurement, + gap, + className, + style, + 'data-testid': dataTestId, + 'data-chart-id': dataChartId, + }, + ref + ) => { + const visibilityStyle = + isWaitingForMeasurement !== undefined + ? { visibility: ( isWaitingForMeasurement ? 'hidden' : 'visible' ) as const } + : {}; + + return ( + + { legendPosition === 'top' && legendElement } + { renderLegendSlot( legendChildren, 'top' ) } + + { children } + + { legendPosition === 'bottom' && legendElement } + { renderLegendSlot( legendChildren, 'bottom' ) } + + { trailingContent } + + ); + } +); + +ChartLayout.displayName = 'ChartLayout'; +``` + +Note: All existing charts import `Stack` from `@wordpress/ui`. + +- [ ] **Step 5: Create the SCSS module** + +```scss +// chart-layout.module.scss +// Intentionally minimal — ChartLayout is a structural wrapper. +// Chart-specific styles remain in each chart's own SCSS module. +.chart-layout { + // Base styles if needed in future. Currently the Stack handles layout. +} +``` + +- [ ] **Step 6: Create the barrel export** + +```ts +// index.ts +export { ChartLayout } from './chart-layout'; +``` + +- [ ] **Step 7: Run the tests** + +Run: `cd projects/js-packages/charts && pnpm test -- --testPathPattern="chart-layout" --no-coverage` +Expected: All tests PASS + +- [ ] **Step 8: Fix any test failures and iterate until green** + +Adjust the implementation if needed. Stack comes from `@wordpress/ui`. Match the `gap` prop type to Stack's expected type (e.g., spacing scale union) rather than plain `string` for tighter typing. + +- [ ] **Step 9: Commit** + +```bash +git add projects/js-packages/charts/src/components/chart-layout/ +git commit -m "Charts: Add ChartLayout component for shared chart+legend layout" +``` + +--- + +## Chunk 2: Migrate Line & Bar Charts + +### Task 3: Refactor LineChart to use ChartLayout + +**Files:** +- Modify: `src/charts/line-chart/line-chart.tsx:468-663` (the return block) + +- [ ] **Step 10: Run existing LineChart tests to establish baseline** + +Run: `cd projects/js-packages/charts && pnpm test -- --testPathPattern="line-chart" --no-coverage` +Expected: All PASS + +- [ ] **Step 11: Refactor LineChart return statement** + +Replace the Stack wrapper in LineChart with ChartLayout. The refactored return should look like: + +```tsx +return ( + + +
+ { ! isWaitingForMeasurement && ( +
+ + { /* ... existing chart internals unchanged ... */ } + +
+ ) } +
+ + +); +``` + +Key changes: +- Remove the outer `` and replace with `` +- Remove the 4 legend lines (top/bottom legendElement + renderLegendSlot calls) +- Remove `{ nonLegendChildren }` from end — pass as `trailingContent` +- Remove `visibility` from `style` — handled by `isWaitingForMeasurement` prop +- Add `import { ChartLayout } from '../../../components/chart-layout'` +- Remove `import { Stack }` if no longer used elsewhere in the file +- Remove `import { renderLegendSlot }` if no longer used + +- [ ] **Step 12: Run LineChart tests** + +Run: `cd projects/js-packages/charts && pnpm test -- --testPathPattern="line-chart" --no-coverage` +Expected: All PASS — behavior unchanged + +- [ ] **Step 13: Commit** + +```bash +git add projects/js-packages/charts/src/charts/line-chart/line-chart.tsx +git commit -m "Charts: Migrate LineChart to use ChartLayout" +``` + +--- + +### Task 4: Refactor BarChart to use ChartLayout + +**Files:** +- Modify: `src/charts/bar-chart/bar-chart.tsx:341-489` (the return block) + +- [ ] **Step 14: Run existing BarChart tests to establish baseline** + +Run: `cd projects/js-packages/charts && pnpm test -- --testPathPattern="bar-chart" --no-coverage` +Expected: All PASS + +- [ ] **Step 15: Refactor BarChart return statement** + +Same pattern as LineChart. Replace Stack with ChartLayout: + +```tsx +return ( + + +
+ { ! isWaitingForMeasurement && ( +
+ + { /* ... unchanged ... */ } + +
+ ) } +
+ + +); +``` + +- [ ] **Step 16: Run BarChart tests** + +Run: `cd projects/js-packages/charts && pnpm test -- --testPathPattern="bar-chart" --no-coverage` +Expected: All PASS + +- [ ] **Step 17: Commit** + +```bash +git add projects/js-packages/charts/src/charts/bar-chart/bar-chart.tsx +git commit -m "Charts: Migrate BarChart to use ChartLayout" +``` + +--- + +## Chunk 3: Migrate Pie Charts & Leaderboard + +### Task 5: Refactor PieChart to use ChartLayout + +**Files:** +- Modify: `src/charts/pie-chart/pie-chart.tsx:327-500` + +- [ ] **Step 18: Run existing PieChart tests to establish baseline** + +Run: `cd projects/js-packages/charts && pnpm test -- --testPathPattern="pie-chart\\.test" --no-coverage` +Expected: All PASS + +- [ ] **Step 19: Refactor PieChart return statement** + +PieChart differences from Line/Bar: +- Uses `containerRef` on the Stack (pass as `ref`) +- No `isWaitingForMeasurement` (omit the prop) +- Has tooltip portal + htmlChildren + otherChildren after bottom legend (combine into `trailingContent`) + +```tsx +return ( + + + { withTooltips && tooltipOpen && tooltipData && ( + +
{ renderTooltip( { tooltipData } ) }
+ + ) } + { htmlChildren } + { otherChildren } + + } + > +
+ + { /* ... unchanged ... */ } + +
+ + +); +``` + +- [ ] **Step 20: Run PieChart tests** + +Run: `cd projects/js-packages/charts && pnpm test -- --testPathPattern="pie-chart\\.test" --no-coverage` +Expected: All PASS + +- [ ] **Step 21: Commit** + +```bash +git add projects/js-packages/charts/src/charts/pie-chart/pie-chart.tsx +git commit -m "Charts: Migrate PieChart to use ChartLayout" +``` + +--- + +### Task 6: Refactor PieSemiCircleChart to use ChartLayout + +**Files:** +- Modify: `src/charts/pie-semi-circle-chart/pie-semi-circle-chart.tsx:359-499` + +- [ ] **Step 22: Run existing PieSemiCircleChart tests** + +Run: `cd projects/js-packages/charts && pnpm test -- --testPathPattern="pie-semi-circle" --no-coverage` +Expected: All PASS + +- [ ] **Step 23: Refactor PieSemiCircleChart return statement** + +Same approach as PieChart — uses `containerRef`, no `isWaitingForMeasurement`, trailing tooltip/html/other content. + +```tsx +return ( + + + { withTooltips && tooltipOpen && tooltipData && ( + +
{ renderTooltip( { tooltipData } ) }
+ + ) } + { htmlChildren } + { otherChildren } + + } + > +
+ + { /* ... unchanged ... */ } + +
+ + +); +``` + +- [ ] **Step 24: Run PieSemiCircleChart tests** + +Run: `cd projects/js-packages/charts && pnpm test -- --testPathPattern="pie-semi-circle" --no-coverage` +Expected: All PASS + +- [ ] **Step 25: Commit** + +```bash +git add projects/js-packages/charts/src/charts/pie-semi-circle-chart/pie-semi-circle-chart.tsx +git commit -m "Charts: Migrate PieSemiCircleChart to use ChartLayout" +``` + +--- + +### Task 7: Refactor LeaderboardChart to use ChartLayout + +**Files:** +- Modify: `src/charts/leaderboard-chart/leaderboard-chart.tsx:299-383` + +- [ ] **Step 26: Run existing LeaderboardChart tests** + +Run: `cd projects/js-packages/charts && pnpm test -- --testPathPattern="leaderboard" --no-coverage` +Expected: All PASS + +- [ ] **Step 27: Refactor LeaderboardChart return statement** + +LeaderboardChart differences: +- No `isWaitingForMeasurement` +- No ref on the Stack +- Has `style` spread (`...style`) +- Uses `data-testid="leaderboard-chart-container"` + +```tsx +return ( + + +
+ { /* ... existing Grid content unchanged ... */ } +
+ + +); +``` + +Also refactor the **empty state** return (around line 260-280) to use ChartLayout for consistency: + +```tsx +return ( + + +
+ { loading + ? __( 'Loading\u2026', 'jetpack-charts' ) + : __( 'No data available', 'jetpack-charts' ) } +
+ + +); +``` + +- [ ] **Step 28: Run LeaderboardChart tests** + +Run: `cd projects/js-packages/charts && pnpm test -- --testPathPattern="leaderboard" --no-coverage` +Expected: All PASS + +- [ ] **Step 29: Commit** + +```bash +git add projects/js-packages/charts/src/charts/leaderboard-chart/leaderboard-chart.tsx +git commit -m "Charts: Migrate LeaderboardChart to use ChartLayout" +``` + +--- + +## Chunk 4: Final Verification + +### Task 8: Run full test suite & Storybook verification + +- [ ] **Step 30: Run full charts test suite** + +Run: `cd projects/js-packages/charts && pnpm test --no-coverage` +Expected: All tests PASS + +- [ ] **Step 31: Type check** + +Run: `cd projects/js-packages/charts && pnpm run build` +Expected: No type errors + +- [ ] **Step 32: Verify Storybook renders** + +Manually check in Storybook that all 5 chart types render correctly with legends at both positions. Check: +- LineChart with legend top/bottom +- BarChart with legend top/bottom +- PieChart with legend top/bottom +- PieSemiCircleChart with legend top/bottom +- LeaderboardChart with legend top/bottom + +- [ ] **Step 33: Clean up unused imports** + +In each migrated chart file, remove any now-unused imports: +- `Stack` from `@wordpress/components` (if only used for the outer wrapper) +- `renderLegendSlot` from `chart-composition` (if only used in the removed pattern) + +Run tests again after cleanup. + +- [ ] **Step 34: Final commit** + +```bash +git add -u projects/js-packages/charts/ +git commit -m "Charts: Clean up unused imports after ChartLayout migration" +``` + +--- + +## Design Decisions + +1. **ChartLayout does NOT create the legend element.** Each chart has slightly different legend defaults (shape: 'rect' vs 'circle', custom className, custom shapeStyles). Keeping legend creation in each chart avoids a complex config object and keeps ChartLayout focused on layout. + +2. **`trailingContent` prop instead of multiple named slots.** Charts have varying needs after the bottom legend (nonLegendChildren, htmlChildren, tooltips). A single `trailingContent` ReactNode keeps the API simple — each chart composes what it needs. + +3. **`isWaitingForMeasurement` is opt-in.** Only Line/Bar charts use visibility hiding for measurement. When `undefined`, no visibility style is applied (Pie/Leaderboard behavior). + +4. **Legend slots use existing `renderLegendSlot` function.** No need to reinvent — ChartLayout calls it the same way each chart did. The composition-API legend children pattern added since the issue was created is fully supported. + +5. **No SCSS for layout itself.** The Stack component handles flex direction/gap. ChartLayout's SCSS is a placeholder for future needs. Chart-specific styles remain in each chart's module. + +6. **`containerRef` on ChartLayout is a known variance (Pie charts only).** PieChart and PieSemiCircleChart pass `containerRef` (from `useTooltipInPortal`) as the `ref` to ChartLayout, while Line/Bar/Leaderboard don't use a ref. Moving tooltip portal bounds measurement to the svg-wrapper div would eliminate this variance but requires merging two refs (`containerRef` + `svgWrapperRef`) and risks tooltip positioning regressions. Tracked as follow-up: [CHARTS-186](https://linear.app/a8c/issue/CHARTS-186/move-tooltip-portal-containerref-off-chartlayout-in-pie-charts). diff --git a/projects/js-packages/charts/src/charts/bar-chart/bar-chart.module.scss b/projects/js-packages/charts/src/charts/bar-chart/bar-chart.module.scss index 363d1758584a..fd2246be150d 100644 --- a/projects/js-packages/charts/src/charts/bar-chart/bar-chart.module.scss +++ b/projects/js-packages/charts/src/charts/bar-chart/bar-chart.module.scss @@ -1,10 +1,5 @@ .bar-chart { - &__svg-wrapper { - flex: 1; - min-height: 0; // Required for flex shrinking - } - svg { overflow: visible; } diff --git a/projects/js-packages/charts/src/charts/bar-chart/bar-chart.tsx b/projects/js-packages/charts/src/charts/bar-chart/bar-chart.tsx index 7c8929f99bf7..bc309e973243 100644 --- a/projects/js-packages/charts/src/charts/bar-chart/bar-chart.tsx +++ b/projects/js-packages/charts/src/charts/bar-chart/bar-chart.tsx @@ -357,14 +357,12 @@ const BarChartInternal: FC< BarChartProps > = ( { data-chart-id={ `bar-chart-${ chartId }` } trailingContent={ nonLegendChildren } > - { ( { contentRef: measureRef, contentHeight } ) => { + { ( { contentHeight } ) => { const chartHeight = contentHeight > 0 ? contentHeight : height; const isWaitingForMeasurement = hasLegend ? contentHeight === 0 : ! chartHeight; return (
( data-testid="line-chart" trailingContent={ nonLegendChildren } > - { ( { contentRef: measureRef, contentHeight } ) => { + { ( { contentHeight } ) => { // Use the measured height, falling back to the passed height if provided. const chartHeight = contentHeight > 0 ? contentHeight : height; const isWaitingForMeasurement = hasLegend ? contentHeight === 0 : ! chartHeight; return (
} > - { ( { contentRef: measureRef, contentWidth, contentHeight } ) => { + { ( { contentWidth, contentHeight } ) => { const availableWidth = contentWidth > 0 ? contentWidth : 300; const availableHeight = contentHeight > 0 ? contentHeight : 300; const availableSize = Math.min( availableWidth, availableHeight ); @@ -346,7 +347,7 @@ const PieChartInternal = ( { : 0; return ( -
+ -
+ ); } } diff --git a/projects/js-packages/charts/src/charts/pie-semi-circle-chart/pie-semi-circle-chart.module.scss b/projects/js-packages/charts/src/charts/pie-semi-circle-chart/pie-semi-circle-chart.module.scss index de49033a51ec..d5ad315647b5 100644 --- a/projects/js-packages/charts/src/charts/pie-semi-circle-chart/pie-semi-circle-chart.module.scss +++ b/projects/js-packages/charts/src/charts/pie-semi-circle-chart/pie-semi-circle-chart.module.scss @@ -5,17 +5,6 @@ width: 100%; } - // Flex wrapper that fills remaining Stack space and measures the SVG area - &__svg-wrapper { - flex: 1; - min-height: 0; // Required for flex shrinking - min-width: 0; // Required for flex shrinking - width: 100%; - display: flex; - align-items: center; - justify-content: center; - } - .label { font-weight: 600; font-size: 16px; diff --git a/projects/js-packages/charts/src/charts/pie-semi-circle-chart/pie-semi-circle-chart.tsx b/projects/js-packages/charts/src/charts/pie-semi-circle-chart/pie-semi-circle-chart.tsx index d3b0b53fb71a..7e57066a0f2d 100644 --- a/projects/js-packages/charts/src/charts/pie-semi-circle-chart/pie-semi-circle-chart.tsx +++ b/projects/js-packages/charts/src/charts/pie-semi-circle-chart/pie-semi-circle-chart.tsx @@ -3,6 +3,7 @@ import { Pie } from '@visx/shape'; import { Text } from '@visx/text'; import { useTooltip, useTooltipInPortal } from '@visx/tooltip'; import { __ } from '@wordpress/i18n'; +import { Stack } from '@wordpress/ui'; import clsx from 'clsx'; import { useCallback, useContext, useMemo } from 'react'; import { ChartLayout } from '../../components/chart-layout'; @@ -370,7 +371,7 @@ const PieSemiCircleChartInternal: FC< PieSemiCircleChartProps > = ( { } > - { ( { contentRef: measureRef, contentWidth, contentHeight } ) => { + { ( { contentWidth, contentHeight } ) => { // Calculate chart dimensions maintaining the 2:1 width-to-height ratio. // Use measured dimensions to respect height constraints, falling back // to explicit props during initial render before measurement is available. @@ -384,7 +385,7 @@ const PieSemiCircleChartInternal: FC< PieSemiCircleChartProps > = ( { const innerRadius = radius * ( 1 - thickness ); return ( -
+ = ( { ) } -
+ ); } } diff --git a/projects/js-packages/charts/src/components/chart-layout/chart-layout.module.scss b/projects/js-packages/charts/src/components/chart-layout/chart-layout.module.scss index 0f3fd3c9a081..25ba6d73455f 100644 --- a/projects/js-packages/charts/src/components/chart-layout/chart-layout.module.scss +++ b/projects/js-packages/charts/src/components/chart-layout/chart-layout.module.scss @@ -1,5 +1,7 @@ -// Intentionally minimal — ChartLayout is a structural wrapper. -// Chart-specific styles remain in each chart's own SCSS module. -.chart-layout { - // Base styles if needed in future. Currently the Stack handles layout. +// Shared content area for charts using the render prop. +// Wraps render prop output and handles measurement. +.chart-layout__content { + flex: 1; + min-height: 0; + min-width: 0; } diff --git a/projects/js-packages/charts/src/components/chart-layout/chart-layout.tsx b/projects/js-packages/charts/src/components/chart-layout/chart-layout.tsx index 9171c53ca534..614fe5984966 100644 --- a/projects/js-packages/charts/src/components/chart-layout/chart-layout.tsx +++ b/projects/js-packages/charts/src/components/chart-layout/chart-layout.tsx @@ -1,5 +1,4 @@ import { Stack } from '@wordpress/ui'; -import clsx from 'clsx'; import { forwardRef } from 'react'; import { renderLegendSlot } from '../../charts/private/chart-composition'; import { useElementSize } from '../../hooks'; @@ -13,8 +12,6 @@ import type { CSSProperties, ReactNode } from 'react'; * Measurements provided to the render prop when ChartLayout handles resize listening. */ export interface ContentMeasurements { - /** Ref callback to attach to the content element being measured */ - contentRef: ( node: HTMLDivElement | null ) => void; /** Measured width of the content area in pixels */ contentWidth: number; /** Measured height of the content area in pixels */ @@ -84,17 +81,17 @@ export const ChartLayout = forwardRef< HTMLDivElement, ChartLayoutProps >( }; } - const renderedChildren = - typeof children === 'function' - ? children( { contentRef, contentWidth, contentHeight, isMeasured } ) - : children; + const isRenderProp = typeof children === 'function'; + const renderedChildren = isRenderProp + ? children( { contentWidth, contentHeight, isMeasured } ) + : children; return ( ( { legendPosition === 'top' && legendElement } { renderLegendSlot( legendChildren, 'top' ) } - { renderedChildren } + { isRenderProp ? ( +
+ { renderedChildren } +
+ ) : ( + renderedChildren + ) } { legendPosition === 'bottom' && legendElement } { renderLegendSlot( legendChildren, 'bottom' ) } From 9496b45b78041580565f3895c8b663f8212568f3 Mon Sep 17 00:00:00 2001 From: Adam Wood <1017872+adamwoodnz@users.noreply.github.com> Date: Thu, 12 Mar 2026 17:01:08 +1300 Subject: [PATCH 16/37] refactor(charts): Remove redundant flex and inline styles from pie charts The pie-chart class had `display: flex; flex-direction: column; align-items: center` which duplicated what the Stack component already provides. The `align-items: center` also prevented the chart-layout content area from stretching to full width. Similarly, the centering Stack inside both pie charts had inline `style={{ width: '100%', height: '100%' }}` that is unnecessary now that ChartLayout's content wrapper handles flex sizing. Also removes hardcoded `size` props from two pie chart stories that were masking the responsive sizing behavior. Co-Authored-By: Claude Opus 4.6 --- .../charts/src/charts/pie-chart/pie-chart.module.scss | 4 +--- .../js-packages/charts/src/charts/pie-chart/pie-chart.tsx | 2 +- .../charts/src/charts/pie-chart/stories/index.stories.tsx | 2 -- .../charts/pie-semi-circle-chart/pie-semi-circle-chart.tsx | 2 +- 4 files changed, 3 insertions(+), 7 deletions(-) diff --git a/projects/js-packages/charts/src/charts/pie-chart/pie-chart.module.scss b/projects/js-packages/charts/src/charts/pie-chart/pie-chart.module.scss index 3cb63f7a40ff..b74139567de3 100644 --- a/projects/js-packages/charts/src/charts/pie-chart/pie-chart.module.scss +++ b/projects/js-packages/charts/src/charts/pie-chart/pie-chart.module.scss @@ -1,12 +1,10 @@ .pie-chart { - display: flex; - flex-direction: column; overflow: hidden; - align-items: center; // Fill parent when no explicit width/height provided &--responsive { height: 100%; width: 100%; } + } diff --git a/projects/js-packages/charts/src/charts/pie-chart/pie-chart.tsx b/projects/js-packages/charts/src/charts/pie-chart/pie-chart.tsx index e8643a5d82ef..6b248cfbb296 100644 --- a/projects/js-packages/charts/src/charts/pie-chart/pie-chart.tsx +++ b/projects/js-packages/charts/src/charts/pie-chart/pie-chart.tsx @@ -347,7 +347,7 @@ const PieChartInternal = ( { : 0; return ( - + = ( { const innerRadius = radius * ( 1 - thickness ); return ( - + Date: Thu, 12 Mar 2026 17:11:12 +1300 Subject: [PATCH 17/37] chore(charts): Remove superpowers plan from tracked files Superpowers directories are now ignored via .git/info/exclude. Removing the previously tracked plan file so it stays local. Made-with: Cursor --- .../plans/2026-03-12-chart-layout.md | 825 ------------------ 1 file changed, 825 deletions(-) delete mode 100644 projects/js-packages/charts/docs/superpowers/plans/2026-03-12-chart-layout.md diff --git a/projects/js-packages/charts/docs/superpowers/plans/2026-03-12-chart-layout.md b/projects/js-packages/charts/docs/superpowers/plans/2026-03-12-chart-layout.md deleted file mode 100644 index 3be4884905c2..000000000000 --- a/projects/js-packages/charts/docs/superpowers/plans/2026-03-12-chart-layout.md +++ /dev/null @@ -1,825 +0,0 @@ -# ChartLayout Component Implementation Plan - -> **For agentic workers:** REQUIRED: Use superpowers:subagent-driven-development (if subagents available) or superpowers:executing-plans to implement this plan. Steps use checkbox (`- [ ]`) syntax for tracking. - -**Goal:** Extract a shared `ChartLayout` component that encapsulates the duplicated Stack + legend positioning pattern across all 5 chart components. - -**Architecture:** ChartLayout is a thin layout wrapper that renders a `Stack direction="column"` with legend slots (top/bottom) sandwiching chart content passed as children. Each chart still owns its own measurement logic, chart rendering, legend element creation, and a11y handlers. ChartLayout handles only the structural layout concern. - -**Tech Stack:** React, TypeScript, SCSS modules, existing Stack/Legend components - -**Linear:** CHARTS-181 - ---- - -## File Structure - -| File | Responsibility | -|------|---------------| -| `src/components/chart-layout/chart-layout.tsx` | ChartLayout component | -| `src/components/chart-layout/chart-layout.module.scss` | Minimal layout styles (visibility hiding) | -| `src/components/chart-layout/index.ts` | Barrel export | -| `src/components/chart-layout/test/chart-layout.test.tsx` | Unit tests | -| `src/charts/line-chart/line-chart.tsx` | Refactor to use ChartLayout | -| `src/charts/bar-chart/bar-chart.tsx` | Refactor to use ChartLayout | -| `src/charts/pie-chart/pie-chart.tsx` | Refactor to use ChartLayout | -| `src/charts/pie-semi-circle-chart/pie-semi-circle-chart.tsx` | Refactor to use ChartLayout | -| `src/charts/leaderboard-chart/leaderboard-chart.tsx` | Refactor to use ChartLayout | - -All paths relative to `projects/js-packages/charts/`. - ---- - -## Chunk 1: ChartLayout Component - -### Task 1: Create ChartLayout test file - -**Files:** -- Create: `src/components/chart-layout/test/chart-layout.test.tsx` - -- [ ] **Step 1: Write failing tests for ChartLayout** - -```tsx -import { render, screen } from '@testing-library/react'; -import { ChartLayout } from '../chart-layout'; -import { renderLegendSlot } from '../../../charts/private/chart-composition'; -import type { LegendChild } from '../../../charts/private/chart-composition/use-chart-children'; - -// Mock renderLegendSlot since we test it separately -jest.mock( '../../../charts/private/chart-composition', () => ( { - renderLegendSlot: jest.fn( () => [] ), -} ) ); - -const mockRenderLegendSlot = renderLegendSlot as jest.Mock; - -describe( 'ChartLayout', () => { - beforeEach( () => { - mockRenderLegendSlot.mockReturnValue( [] ); - } ); - - it( 'renders children inside a column Stack', () => { - render( - -
Chart
- - ); - expect( screen.getByTestId( 'chart-content' ) ).toBeInTheDocument(); - } ); - - it( 'renders legend element at top when legendPosition is top', () => { - const legendElement =
Legend
; - const { container } = render( - -
Chart
- - ); - const stack = container.firstChild; - const legend = screen.getByTestId( 'legend' ); - const content = screen.getByTestId( 'chart-content' ); - // Legend should come before content in DOM order - expect( - legend.compareDocumentPosition( content ) & Node.DOCUMENT_POSITION_FOLLOWING - ).toBeTruthy(); - } ); - - it( 'renders legend element at bottom when legendPosition is bottom', () => { - const legendElement =
Legend
; - render( - -
Chart
- - ); - const legend = screen.getByTestId( 'legend' ); - const content = screen.getByTestId( 'chart-content' ); - // Content should come before legend in DOM order - expect( - content.compareDocumentPosition( legend ) & Node.DOCUMENT_POSITION_FOLLOWING - ).toBeTruthy(); - } ); - - it( 'does not render legend element when it is false/null', () => { - render( - -
Chart
- - ); - expect( screen.queryByTestId( 'legend' ) ).not.toBeInTheDocument(); - } ); - - it( 'calls renderLegendSlot for both positions', () => { - const legendChildren: LegendChild[] = []; - render( - -
Chart
- - ); - expect( mockRenderLegendSlot ).toHaveBeenCalledWith( legendChildren, 'top' ); - expect( mockRenderLegendSlot ).toHaveBeenCalledWith( legendChildren, 'bottom' ); - } ); - - it( 'applies visibility hidden when isWaitingForMeasurement is true', () => { - render( - -
Chart
- - ); - expect( screen.getByTestId( 'layout' ) ).toHaveStyle( { visibility: 'hidden' } ); - } ); - - it( 'applies visibility visible when isWaitingForMeasurement is false', () => { - render( - -
Chart
- - ); - expect( screen.getByTestId( 'layout' ) ).toHaveStyle( { visibility: 'visible' } ); - } ); - - it( 'does not apply visibility style when isWaitingForMeasurement is undefined', () => { - render( - -
Chart
- - ); - // No visibility in style when not opted in - expect( screen.getByTestId( 'layout' ).style.visibility ).toBe( '' ); - } ); - - it( 'passes className and style to Stack', () => { - render( - -
Chart
- - ); - const layout = screen.getByTestId( 'layout' ); - expect( layout ).toHaveClass( 'my-chart' ); - expect( layout ).toHaveStyle( { width: '400px', height: '300px' } ); - } ); - - it( 'passes gap to Stack', () => { - const { container } = render( - -
Chart
- - ); - // Stack renders gap as a CSS class or style — just verify it renders without error - expect( container.firstChild ).toBeInTheDocument(); - } ); - - it( 'forwards ref to Stack', () => { - const ref = jest.fn(); - render( - -
Chart
- - ); - expect( ref ).toHaveBeenCalledWith( expect.any( HTMLElement ) ); - } ); - - it( 'renders trailing content after bottom legend', () => { - render( - Legend
} - legendChildren={ [] } - trailingContent={
Extra
} - > -
Chart
- - ); - const legend = screen.getByTestId( 'legend' ); - const trailing = screen.getByTestId( 'trailing' ); - expect( - legend.compareDocumentPosition( trailing ) & Node.DOCUMENT_POSITION_FOLLOWING - ).toBeTruthy(); - } ); -} ); -``` - -- [ ] **Step 2: Run tests to verify they fail** - -Run: `cd projects/js-packages/charts && pnpm test -- --testPathPattern="chart-layout" --no-coverage` -Expected: FAIL — module not found - -- [ ] **Step 3: Commit test file** - -```bash -git add projects/js-packages/charts/src/components/chart-layout/test/chart-layout.test.tsx -git commit -m "Charts: Add ChartLayout component tests (red phase)" -``` - ---- - -### Task 2: Implement ChartLayout component - -**Files:** -- Create: `src/components/chart-layout/chart-layout.tsx` -- Create: `src/components/chart-layout/chart-layout.module.scss` -- Create: `src/components/chart-layout/index.ts` - -- [ ] **Step 4: Create the ChartLayout component** - -```tsx -// chart-layout.tsx -import clsx from 'clsx'; -import { forwardRef } from 'react'; -import { Stack } from '@wordpress/ui'; -import { renderLegendSlot } from '../../charts/private/chart-composition'; -import type { LegendChild } from '../../charts/private/chart-composition/use-chart-children'; -import type { LegendPosition } from '../../types'; -import type { CSSProperties, ReactNode } from 'react'; -import styles from './chart-layout.module.scss'; - -interface ChartLayoutProps { - /** Position for the prop-based legend element */ - legendPosition: LegendPosition; - /** The legend element rendered via the showLegend prop (false when hidden) */ - legendElement?: ReactNode; - /** Legend children from the composition API */ - legendChildren: LegendChild[]; - /** Chart content rendered between legend slots */ - children: ReactNode; - /** Content rendered after the bottom legend (e.g., nonLegendChildren, htmlChildren, tooltips) */ - trailingContent?: ReactNode; - /** When true, sets visibility: hidden on the container. Used by charts that need measurement before rendering. */ - isWaitingForMeasurement?: boolean; - /** Gap between Stack items */ - gap?: string; - /** Additional class names */ - className?: string; - /** Inline styles (width, height, etc.) */ - style?: CSSProperties; - /** Test ID for the container */ - 'data-testid'?: string; - /** Chart ID attribute */ - 'data-chart-id'?: string; -} - -export const ChartLayout = forwardRef< HTMLDivElement, ChartLayoutProps >( - ( - { - legendPosition, - legendElement, - legendChildren, - children, - trailingContent, - isWaitingForMeasurement, - gap, - className, - style, - 'data-testid': dataTestId, - 'data-chart-id': dataChartId, - }, - ref - ) => { - const visibilityStyle = - isWaitingForMeasurement !== undefined - ? { visibility: ( isWaitingForMeasurement ? 'hidden' : 'visible' ) as const } - : {}; - - return ( - - { legendPosition === 'top' && legendElement } - { renderLegendSlot( legendChildren, 'top' ) } - - { children } - - { legendPosition === 'bottom' && legendElement } - { renderLegendSlot( legendChildren, 'bottom' ) } - - { trailingContent } - - ); - } -); - -ChartLayout.displayName = 'ChartLayout'; -``` - -Note: All existing charts import `Stack` from `@wordpress/ui`. - -- [ ] **Step 5: Create the SCSS module** - -```scss -// chart-layout.module.scss -// Intentionally minimal — ChartLayout is a structural wrapper. -// Chart-specific styles remain in each chart's own SCSS module. -.chart-layout { - // Base styles if needed in future. Currently the Stack handles layout. -} -``` - -- [ ] **Step 6: Create the barrel export** - -```ts -// index.ts -export { ChartLayout } from './chart-layout'; -``` - -- [ ] **Step 7: Run the tests** - -Run: `cd projects/js-packages/charts && pnpm test -- --testPathPattern="chart-layout" --no-coverage` -Expected: All tests PASS - -- [ ] **Step 8: Fix any test failures and iterate until green** - -Adjust the implementation if needed. Stack comes from `@wordpress/ui`. Match the `gap` prop type to Stack's expected type (e.g., spacing scale union) rather than plain `string` for tighter typing. - -- [ ] **Step 9: Commit** - -```bash -git add projects/js-packages/charts/src/components/chart-layout/ -git commit -m "Charts: Add ChartLayout component for shared chart+legend layout" -``` - ---- - -## Chunk 2: Migrate Line & Bar Charts - -### Task 3: Refactor LineChart to use ChartLayout - -**Files:** -- Modify: `src/charts/line-chart/line-chart.tsx:468-663` (the return block) - -- [ ] **Step 10: Run existing LineChart tests to establish baseline** - -Run: `cd projects/js-packages/charts && pnpm test -- --testPathPattern="line-chart" --no-coverage` -Expected: All PASS - -- [ ] **Step 11: Refactor LineChart return statement** - -Replace the Stack wrapper in LineChart with ChartLayout. The refactored return should look like: - -```tsx -return ( - - -
- { ! isWaitingForMeasurement && ( -
- - { /* ... existing chart internals unchanged ... */ } - -
- ) } -
- - -); -``` - -Key changes: -- Remove the outer `` and replace with `` -- Remove the 4 legend lines (top/bottom legendElement + renderLegendSlot calls) -- Remove `{ nonLegendChildren }` from end — pass as `trailingContent` -- Remove `visibility` from `style` — handled by `isWaitingForMeasurement` prop -- Add `import { ChartLayout } from '../../../components/chart-layout'` -- Remove `import { Stack }` if no longer used elsewhere in the file -- Remove `import { renderLegendSlot }` if no longer used - -- [ ] **Step 12: Run LineChart tests** - -Run: `cd projects/js-packages/charts && pnpm test -- --testPathPattern="line-chart" --no-coverage` -Expected: All PASS — behavior unchanged - -- [ ] **Step 13: Commit** - -```bash -git add projects/js-packages/charts/src/charts/line-chart/line-chart.tsx -git commit -m "Charts: Migrate LineChart to use ChartLayout" -``` - ---- - -### Task 4: Refactor BarChart to use ChartLayout - -**Files:** -- Modify: `src/charts/bar-chart/bar-chart.tsx:341-489` (the return block) - -- [ ] **Step 14: Run existing BarChart tests to establish baseline** - -Run: `cd projects/js-packages/charts && pnpm test -- --testPathPattern="bar-chart" --no-coverage` -Expected: All PASS - -- [ ] **Step 15: Refactor BarChart return statement** - -Same pattern as LineChart. Replace Stack with ChartLayout: - -```tsx -return ( - - -
- { ! isWaitingForMeasurement && ( -
- - { /* ... unchanged ... */ } - -
- ) } -
- - -); -``` - -- [ ] **Step 16: Run BarChart tests** - -Run: `cd projects/js-packages/charts && pnpm test -- --testPathPattern="bar-chart" --no-coverage` -Expected: All PASS - -- [ ] **Step 17: Commit** - -```bash -git add projects/js-packages/charts/src/charts/bar-chart/bar-chart.tsx -git commit -m "Charts: Migrate BarChart to use ChartLayout" -``` - ---- - -## Chunk 3: Migrate Pie Charts & Leaderboard - -### Task 5: Refactor PieChart to use ChartLayout - -**Files:** -- Modify: `src/charts/pie-chart/pie-chart.tsx:327-500` - -- [ ] **Step 18: Run existing PieChart tests to establish baseline** - -Run: `cd projects/js-packages/charts && pnpm test -- --testPathPattern="pie-chart\\.test" --no-coverage` -Expected: All PASS - -- [ ] **Step 19: Refactor PieChart return statement** - -PieChart differences from Line/Bar: -- Uses `containerRef` on the Stack (pass as `ref`) -- No `isWaitingForMeasurement` (omit the prop) -- Has tooltip portal + htmlChildren + otherChildren after bottom legend (combine into `trailingContent`) - -```tsx -return ( - - - { withTooltips && tooltipOpen && tooltipData && ( - -
{ renderTooltip( { tooltipData } ) }
- - ) } - { htmlChildren } - { otherChildren } - - } - > -
- - { /* ... unchanged ... */ } - -
- - -); -``` - -- [ ] **Step 20: Run PieChart tests** - -Run: `cd projects/js-packages/charts && pnpm test -- --testPathPattern="pie-chart\\.test" --no-coverage` -Expected: All PASS - -- [ ] **Step 21: Commit** - -```bash -git add projects/js-packages/charts/src/charts/pie-chart/pie-chart.tsx -git commit -m "Charts: Migrate PieChart to use ChartLayout" -``` - ---- - -### Task 6: Refactor PieSemiCircleChart to use ChartLayout - -**Files:** -- Modify: `src/charts/pie-semi-circle-chart/pie-semi-circle-chart.tsx:359-499` - -- [ ] **Step 22: Run existing PieSemiCircleChart tests** - -Run: `cd projects/js-packages/charts && pnpm test -- --testPathPattern="pie-semi-circle" --no-coverage` -Expected: All PASS - -- [ ] **Step 23: Refactor PieSemiCircleChart return statement** - -Same approach as PieChart — uses `containerRef`, no `isWaitingForMeasurement`, trailing tooltip/html/other content. - -```tsx -return ( - - - { withTooltips && tooltipOpen && tooltipData && ( - -
{ renderTooltip( { tooltipData } ) }
- - ) } - { htmlChildren } - { otherChildren } - - } - > -
- - { /* ... unchanged ... */ } - -
- - -); -``` - -- [ ] **Step 24: Run PieSemiCircleChart tests** - -Run: `cd projects/js-packages/charts && pnpm test -- --testPathPattern="pie-semi-circle" --no-coverage` -Expected: All PASS - -- [ ] **Step 25: Commit** - -```bash -git add projects/js-packages/charts/src/charts/pie-semi-circle-chart/pie-semi-circle-chart.tsx -git commit -m "Charts: Migrate PieSemiCircleChart to use ChartLayout" -``` - ---- - -### Task 7: Refactor LeaderboardChart to use ChartLayout - -**Files:** -- Modify: `src/charts/leaderboard-chart/leaderboard-chart.tsx:299-383` - -- [ ] **Step 26: Run existing LeaderboardChart tests** - -Run: `cd projects/js-packages/charts && pnpm test -- --testPathPattern="leaderboard" --no-coverage` -Expected: All PASS - -- [ ] **Step 27: Refactor LeaderboardChart return statement** - -LeaderboardChart differences: -- No `isWaitingForMeasurement` -- No ref on the Stack -- Has `style` spread (`...style`) -- Uses `data-testid="leaderboard-chart-container"` - -```tsx -return ( - - -
- { /* ... existing Grid content unchanged ... */ } -
- - -); -``` - -Also refactor the **empty state** return (around line 260-280) to use ChartLayout for consistency: - -```tsx -return ( - - -
- { loading - ? __( 'Loading\u2026', 'jetpack-charts' ) - : __( 'No data available', 'jetpack-charts' ) } -
- - -); -``` - -- [ ] **Step 28: Run LeaderboardChart tests** - -Run: `cd projects/js-packages/charts && pnpm test -- --testPathPattern="leaderboard" --no-coverage` -Expected: All PASS - -- [ ] **Step 29: Commit** - -```bash -git add projects/js-packages/charts/src/charts/leaderboard-chart/leaderboard-chart.tsx -git commit -m "Charts: Migrate LeaderboardChart to use ChartLayout" -``` - ---- - -## Chunk 4: Final Verification - -### Task 8: Run full test suite & Storybook verification - -- [ ] **Step 30: Run full charts test suite** - -Run: `cd projects/js-packages/charts && pnpm test --no-coverage` -Expected: All tests PASS - -- [ ] **Step 31: Type check** - -Run: `cd projects/js-packages/charts && pnpm run build` -Expected: No type errors - -- [ ] **Step 32: Verify Storybook renders** - -Manually check in Storybook that all 5 chart types render correctly with legends at both positions. Check: -- LineChart with legend top/bottom -- BarChart with legend top/bottom -- PieChart with legend top/bottom -- PieSemiCircleChart with legend top/bottom -- LeaderboardChart with legend top/bottom - -- [ ] **Step 33: Clean up unused imports** - -In each migrated chart file, remove any now-unused imports: -- `Stack` from `@wordpress/components` (if only used for the outer wrapper) -- `renderLegendSlot` from `chart-composition` (if only used in the removed pattern) - -Run tests again after cleanup. - -- [ ] **Step 34: Final commit** - -```bash -git add -u projects/js-packages/charts/ -git commit -m "Charts: Clean up unused imports after ChartLayout migration" -``` - ---- - -## Design Decisions - -1. **ChartLayout does NOT create the legend element.** Each chart has slightly different legend defaults (shape: 'rect' vs 'circle', custom className, custom shapeStyles). Keeping legend creation in each chart avoids a complex config object and keeps ChartLayout focused on layout. - -2. **`trailingContent` prop instead of multiple named slots.** Charts have varying needs after the bottom legend (nonLegendChildren, htmlChildren, tooltips). A single `trailingContent` ReactNode keeps the API simple — each chart composes what it needs. - -3. **`isWaitingForMeasurement` is opt-in.** Only Line/Bar charts use visibility hiding for measurement. When `undefined`, no visibility style is applied (Pie/Leaderboard behavior). - -4. **Legend slots use existing `renderLegendSlot` function.** No need to reinvent — ChartLayout calls it the same way each chart did. The composition-API legend children pattern added since the issue was created is fully supported. - -5. **No SCSS for layout itself.** The Stack component handles flex direction/gap. ChartLayout's SCSS is a placeholder for future needs. Chart-specific styles remain in each chart's module. - -6. **`containerRef` on ChartLayout is a known variance (Pie charts only).** PieChart and PieSemiCircleChart pass `containerRef` (from `useTooltipInPortal`) as the `ref` to ChartLayout, while Line/Bar/Leaderboard don't use a ref. Moving tooltip portal bounds measurement to the svg-wrapper div would eliminate this variance but requires merging two refs (`containerRef` + `svgWrapperRef`) and risks tooltip positioning regressions. Tracked as follow-up: [CHARTS-186](https://linear.app/a8c/issue/CHARTS-186/move-tooltip-portal-containerref-off-chartlayout-in-pie-charts). From 70d5c01ff1bf97987e91891ebda9dc7b9812411a Mon Sep 17 00:00:00 2001 From: Adam Wood <1017872+adamwoodnz@users.noreply.github.com> Date: Thu, 12 Mar 2026 17:32:21 +1300 Subject: [PATCH 18/37] refactor(charts): Move ChartLayout to charts/private ChartLayout is internal infrastructure consumed only by chart components, not a public API. Moving it alongside other internal modules (chart-composition, single-chart-context, with-responsive) makes its scope clear and avoids exposing it in the components directory. Co-Authored-By: Claude Opus 4.6 --- .../js-packages/charts/src/charts/bar-chart/bar-chart.tsx | 2 +- .../src/charts/leaderboard-chart/leaderboard-chart.tsx | 2 +- .../charts/src/charts/line-chart/line-chart.tsx | 2 +- .../js-packages/charts/src/charts/pie-chart/pie-chart.tsx | 2 +- .../pie-semi-circle-chart/pie-semi-circle-chart.tsx | 2 +- .../private}/chart-layout/chart-layout.module.scss | 0 .../private}/chart-layout/chart-layout.tsx | 8 ++++---- .../{components => charts/private}/chart-layout/index.ts | 0 .../private}/chart-layout/test/chart-layout.test.tsx | 6 +++--- 9 files changed, 12 insertions(+), 12 deletions(-) rename projects/js-packages/charts/src/{components => charts/private}/chart-layout/chart-layout.module.scss (100%) rename projects/js-packages/charts/src/{components => charts/private}/chart-layout/chart-layout.tsx (93%) rename projects/js-packages/charts/src/{components => charts/private}/chart-layout/index.ts (100%) rename projects/js-packages/charts/src/{components => charts/private}/chart-layout/test/chart-layout.test.tsx (95%) diff --git a/projects/js-packages/charts/src/charts/bar-chart/bar-chart.tsx b/projects/js-packages/charts/src/charts/bar-chart/bar-chart.tsx index bc309e973243..47734e89ac63 100644 --- a/projects/js-packages/charts/src/charts/bar-chart/bar-chart.tsx +++ b/projects/js-packages/charts/src/charts/bar-chart/bar-chart.tsx @@ -4,7 +4,6 @@ import { Axis, BarSeries, BarGroup, Grid, XYChart } from '@visx/xychart'; import { __ } from '@wordpress/i18n'; import clsx from 'clsx'; import { useCallback, useContext, useState, useRef, useMemo } from 'react'; -import { ChartLayout } from '../../components/chart-layout'; import { Legend, useChartLegendItems } from '../../components/legend'; import { AccessibleTooltip, useKeyboardNavigation } from '../../components/tooltip'; import { @@ -24,6 +23,7 @@ import { } from '../../providers'; import { attachSubComponents } from '../../utils'; import { useChartChildren } from '../private/chart-composition'; +import { ChartLayout } from '../private/chart-layout'; import { SingleChartContext } from '../private/single-chart-context'; import { withResponsive } from '../private/with-responsive'; import styles from './bar-chart.module.scss'; diff --git a/projects/js-packages/charts/src/charts/leaderboard-chart/leaderboard-chart.tsx b/projects/js-packages/charts/src/charts/leaderboard-chart/leaderboard-chart.tsx index 408052d70adb..06ae954ca4de 100644 --- a/projects/js-packages/charts/src/charts/leaderboard-chart/leaderboard-chart.tsx +++ b/projects/js-packages/charts/src/charts/leaderboard-chart/leaderboard-chart.tsx @@ -5,7 +5,6 @@ import { __ } from '@wordpress/i18n'; import { Stack } from '@wordpress/ui'; import clsx from 'clsx'; import { useContext, useMemo, type FC } from 'react'; -import { ChartLayout } from '../../components/chart-layout'; import { Legend } from '../../components/legend'; import { usePrefersReducedMotion } from '../../hooks'; import { @@ -18,6 +17,7 @@ import { } from '../../providers'; import { formatMetricValue, attachSubComponents } from '../../utils'; import { useChartChildren } from '../private/chart-composition'; +import { ChartLayout } from '../private/chart-layout'; import { SingleChartContext } from '../private/single-chart-context'; import { withResponsive } from '../private/with-responsive'; import { useLeaderboardLegendItems } from './hooks'; diff --git a/projects/js-packages/charts/src/charts/line-chart/line-chart.tsx b/projects/js-packages/charts/src/charts/line-chart/line-chart.tsx index 36cfc910b166..f3a008d4b711 100644 --- a/projects/js-packages/charts/src/charts/line-chart/line-chart.tsx +++ b/projects/js-packages/charts/src/charts/line-chart/line-chart.tsx @@ -7,7 +7,6 @@ import { __ } from '@wordpress/i18n'; import clsx from 'clsx'; import { differenceInHours, differenceInYears } from 'date-fns'; import { useMemo, useContext, forwardRef, useImperativeHandle, useState, useRef } from 'react'; -import { ChartLayout } from '../../components/chart-layout'; import { Legend, useChartLegendItems } from '../../components/legend'; import { AccessibleTooltip, useKeyboardNavigation } from '../../components/tooltip'; import { @@ -26,6 +25,7 @@ import { } from '../../providers'; import { attachSubComponents } from '../../utils'; import { useChartChildren } from '../private/chart-composition'; +import { ChartLayout } from '../private/chart-layout'; import { DefaultGlyph } from '../private/default-glyph'; import { SingleChartContext, type SingleChartRef } from '../private/single-chart-context'; import { withResponsive } from '../private/with-responsive'; diff --git a/projects/js-packages/charts/src/charts/pie-chart/pie-chart.tsx b/projects/js-packages/charts/src/charts/pie-chart/pie-chart.tsx index 6b248cfbb296..4a35a7481e1c 100644 --- a/projects/js-packages/charts/src/charts/pie-chart/pie-chart.tsx +++ b/projects/js-packages/charts/src/charts/pie-chart/pie-chart.tsx @@ -5,7 +5,6 @@ import { __ } from '@wordpress/i18n'; import { Stack } from '@wordpress/ui'; import clsx from 'clsx'; import { useCallback, useContext, useMemo } from 'react'; -import { ChartLayout } from '../../components/chart-layout'; import { Legend, useChartLegendItems } from '../../components/legend'; import { BaseTooltip } from '../../components/tooltip'; import { useInteractiveLegendData, usePrefersReducedMotion } from '../../hooks'; @@ -20,6 +19,7 @@ import { import { attachSubComponents } from '../../utils'; import { getStringWidth } from '../../visx/text'; import { ChartSVG, ChartHTML, useChartChildren } from '../private/chart-composition'; +import { ChartLayout } from '../private/chart-layout'; import { RadialWipeAnimation } from '../private/radial-wipe-animation/'; import { SingleChartContext } from '../private/single-chart-context'; import { withResponsive, ResponsiveConfig } from '../private/with-responsive'; diff --git a/projects/js-packages/charts/src/charts/pie-semi-circle-chart/pie-semi-circle-chart.tsx b/projects/js-packages/charts/src/charts/pie-semi-circle-chart/pie-semi-circle-chart.tsx index 3a6cf2bc1e9c..a576a4c0d9b9 100644 --- a/projects/js-packages/charts/src/charts/pie-semi-circle-chart/pie-semi-circle-chart.tsx +++ b/projects/js-packages/charts/src/charts/pie-semi-circle-chart/pie-semi-circle-chart.tsx @@ -6,7 +6,6 @@ import { __ } from '@wordpress/i18n'; import { Stack } from '@wordpress/ui'; import clsx from 'clsx'; import { useCallback, useContext, useMemo } from 'react'; -import { ChartLayout } from '../../components/chart-layout'; import { Legend, useChartLegendItems } from '../../components/legend'; import { BaseTooltip } from '../../components/tooltip'; import { useInteractiveLegendData, usePrefersReducedMotion } from '../../hooks'; @@ -19,6 +18,7 @@ import { } from '../../providers'; import { attachSubComponents } from '../../utils'; import { ChartSVG, ChartHTML, useChartChildren } from '../private/chart-composition'; +import { ChartLayout } from '../private/chart-layout'; import { RadialWipeAnimation } from '../private/radial-wipe-animation'; import { SingleChartContext } from '../private/single-chart-context'; import { withResponsive } from '../private/with-responsive'; diff --git a/projects/js-packages/charts/src/components/chart-layout/chart-layout.module.scss b/projects/js-packages/charts/src/charts/private/chart-layout/chart-layout.module.scss similarity index 100% rename from projects/js-packages/charts/src/components/chart-layout/chart-layout.module.scss rename to projects/js-packages/charts/src/charts/private/chart-layout/chart-layout.module.scss diff --git a/projects/js-packages/charts/src/components/chart-layout/chart-layout.tsx b/projects/js-packages/charts/src/charts/private/chart-layout/chart-layout.tsx similarity index 93% rename from projects/js-packages/charts/src/components/chart-layout/chart-layout.tsx rename to projects/js-packages/charts/src/charts/private/chart-layout/chart-layout.tsx index 614fe5984966..d325f8a69381 100644 --- a/projects/js-packages/charts/src/components/chart-layout/chart-layout.tsx +++ b/projects/js-packages/charts/src/charts/private/chart-layout/chart-layout.tsx @@ -1,10 +1,10 @@ import { Stack } from '@wordpress/ui'; import { forwardRef } from 'react'; -import { renderLegendSlot } from '../../charts/private/chart-composition'; -import { useElementSize } from '../../hooks'; +import { useElementSize } from '../../../hooks'; +import { renderLegendSlot } from '../chart-composition'; import styles from './chart-layout.module.scss'; -import type { LegendChild } from '../../charts/private/chart-composition/use-chart-children'; -import type { LegendPosition } from '../../types'; +import type { LegendPosition } from '../../../types'; +import type { LegendChild } from '../chart-composition/use-chart-children'; import type { GapSize } from '@wordpress/theme'; import type { CSSProperties, ReactNode } from 'react'; diff --git a/projects/js-packages/charts/src/components/chart-layout/index.ts b/projects/js-packages/charts/src/charts/private/chart-layout/index.ts similarity index 100% rename from projects/js-packages/charts/src/components/chart-layout/index.ts rename to projects/js-packages/charts/src/charts/private/chart-layout/index.ts diff --git a/projects/js-packages/charts/src/components/chart-layout/test/chart-layout.test.tsx b/projects/js-packages/charts/src/charts/private/chart-layout/test/chart-layout.test.tsx similarity index 95% rename from projects/js-packages/charts/src/components/chart-layout/test/chart-layout.test.tsx rename to projects/js-packages/charts/src/charts/private/chart-layout/test/chart-layout.test.tsx index 11fbd8ac371a..4164317a37cf 100644 --- a/projects/js-packages/charts/src/components/chart-layout/test/chart-layout.test.tsx +++ b/projects/js-packages/charts/src/charts/private/chart-layout/test/chart-layout.test.tsx @@ -1,10 +1,10 @@ import { render, screen } from '@testing-library/react'; -import { renderLegendSlot } from '../../../charts/private/chart-composition'; +import { renderLegendSlot } from '../../chart-composition'; import { ChartLayout } from '../chart-layout'; -import type { LegendChild } from '../../../charts/private/chart-composition/use-chart-children'; +import type { LegendChild } from '../../chart-composition/use-chart-children'; // Mock renderLegendSlot since we test it separately -jest.mock( '../../../charts/private/chart-composition', () => ( { +jest.mock( '../../chart-composition', () => ( { renderLegendSlot: jest.fn( () => [] ), } ) ); From 883efdf82188226e3a8b90c8d3892988a2a05b3f Mon Sep 17 00:00:00 2001 From: Adam Wood <1017872+adamwoodnz@users.noreply.github.com> Date: Thu, 12 Mar 2026 17:53:50 +1300 Subject: [PATCH 19/37] fix(charts): Pass measured chart height to SingleChartContext in LineChart SingleChartContext.chartHeight was set to the full container height (including legend space), but AnnotationsOverlay uses this value to size its SVG overlay. This caused misaligned annotations when a legend was present because the overlay was sized larger than the actual chart drawing area. Lift the measured content height from the ChartLayout render prop into state so the context provider reflects the true drawable area. Co-Authored-By: Claude Opus 4.6 --- .../charts/src/charts/line-chart/line-chart.tsx | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/projects/js-packages/charts/src/charts/line-chart/line-chart.tsx b/projects/js-packages/charts/src/charts/line-chart/line-chart.tsx index f3a008d4b711..fec42ba1285b 100644 --- a/projects/js-packages/charts/src/charts/line-chart/line-chart.tsx +++ b/projects/js-packages/charts/src/charts/line-chart/line-chart.tsx @@ -294,6 +294,7 @@ const LineChartInternal = forwardRef< SingleChartRef, LineChartProps >( const hasLegendChild = legendChildren.length > 0; const hasLegend = showLegend || hasLegendChild; + const [ measuredChartHeight, setMeasuredChartHeight ] = useState< number | undefined >(); // Forward the external ref to the internal ref useImperativeHandle( @@ -465,7 +466,7 @@ const LineChartInternal = forwardRef< SingleChartRef, LineChartProps >( chartId, chartRef: internalChartRef, chartWidth: width, - chartHeight: height || 0, + chartHeight: measuredChartHeight || 0, } } > ( const chartHeight = contentHeight > 0 ? contentHeight : height; const isWaitingForMeasurement = hasLegend ? contentHeight === 0 : ! chartHeight; + // Update the measured chart height for context consumers (e.g. AnnotationsOverlay) + if ( chartHeight !== measuredChartHeight ) { + setMeasuredChartHeight( chartHeight ); + } + return (
Date: Thu, 12 Mar 2026 17:54:03 +1300 Subject: [PATCH 20/37] fix(charts): Add full dimensions to pie chart centering Stack The centering Stack inside the ChartLayout render prop had no explicit dimensions, so justify="center" had no effect because the Stack collapsed to content height. Adding width/height 100% makes the Stack fill the measured content area, enabling proper vertical centering. Co-Authored-By: Claude Opus 4.6 --- projects/js-packages/charts/src/charts/pie-chart/pie-chart.tsx | 2 +- .../src/charts/pie-semi-circle-chart/pie-semi-circle-chart.tsx | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/projects/js-packages/charts/src/charts/pie-chart/pie-chart.tsx b/projects/js-packages/charts/src/charts/pie-chart/pie-chart.tsx index 4a35a7481e1c..ab7329848d7a 100644 --- a/projects/js-packages/charts/src/charts/pie-chart/pie-chart.tsx +++ b/projects/js-packages/charts/src/charts/pie-chart/pie-chart.tsx @@ -347,7 +347,7 @@ const PieChartInternal = ( { : 0; return ( - + = ( { const innerRadius = radius * ( 1 - thickness ); return ( - + Date: Thu, 12 Mar 2026 17:54:15 +1300 Subject: [PATCH 21/37] docs(charts): Fix misleading isMeasured docstring in ChartLayout The docstring claimed isMeasured is "always true when waitForMeasurement is not set", but isMeasured derives from contentHeight > 0, which is false on initial render before ResizeObserver fires regardless of waitForMeasurement. Co-Authored-By: Claude Opus 4.6 --- .../charts/src/charts/private/chart-layout/chart-layout.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/projects/js-packages/charts/src/charts/private/chart-layout/chart-layout.tsx b/projects/js-packages/charts/src/charts/private/chart-layout/chart-layout.tsx index d325f8a69381..af7d929f5d98 100644 --- a/projects/js-packages/charts/src/charts/private/chart-layout/chart-layout.tsx +++ b/projects/js-packages/charts/src/charts/private/chart-layout/chart-layout.tsx @@ -16,7 +16,7 @@ export interface ContentMeasurements { contentWidth: number; /** Measured height of the content area in pixels */ contentHeight: number; - /** True once the content area has been measured. Always true when waitForMeasurement is not set. */ + /** True when a non-zero contentHeight measurement is available */ isMeasured: boolean; } From d670ffc21c967b686b8079f2f8a1921bc2b8e274 Mon Sep 17 00:00:00 2001 From: Adam Wood <1017872+adamwoodnz@users.noreply.github.com> Date: Thu, 12 Mar 2026 17:54:48 +1300 Subject: [PATCH 22/37] test(charts): Add ChartLayout render-prop and waitForMeasurement tests Cover the render-prop measurement behavior (function-as-children receives contentWidth/contentHeight/isMeasured) and verify that waitForMeasurement takes precedence over the legacy isWaitingForMeasurement prop. Co-Authored-By: Claude Opus 4.6 --- .../chart-layout/test/chart-layout.test.tsx | 39 +++++++++++++++++++ 1 file changed, 39 insertions(+) diff --git a/projects/js-packages/charts/src/charts/private/chart-layout/test/chart-layout.test.tsx b/projects/js-packages/charts/src/charts/private/chart-layout/test/chart-layout.test.tsx index 4164317a37cf..0c8fd8bf92da 100644 --- a/projects/js-packages/charts/src/charts/private/chart-layout/test/chart-layout.test.tsx +++ b/projects/js-packages/charts/src/charts/private/chart-layout/test/chart-layout.test.tsx @@ -144,6 +144,45 @@ describe( 'ChartLayout', () => { expect( ref ).toHaveBeenCalledWith( expect.any( HTMLElement ) ); } ); + it( 'calls function-as-children with measurement props', () => { + const childFn = jest.fn().mockReturnValue(
Chart
); + + render( + + { childFn } + + ); + + expect( childFn ).toHaveBeenCalled(); + const firstCallArg = childFn.mock.calls[ 0 ][ 0 ]; + + expect( firstCallArg ).toEqual( + expect.objectContaining( { + contentWidth: expect.any( Number ), + contentHeight: expect.any( Number ), + isMeasured: expect.any( Boolean ), + } ) + ); + } ); + + it( 'gives waitForMeasurement precedence over isWaitingForMeasurement', () => { + render( + +
Chart
+ + ); + + // waitForMeasurement=true with no measurement yet should hide, + // even though isWaitingForMeasurement=false would show + expect( screen.getByTestId( 'layout' ) ).toHaveStyle( { visibility: 'hidden' } ); + } ); + it( 'renders trailing content after bottom legend', () => { render( Date: Thu, 12 Mar 2026 17:56:46 +1300 Subject: [PATCH 23/37] refactor(charts): Remove legacy isWaitingForMeasurement prop from ChartLayout MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit No callers pass isWaitingForMeasurement as a prop to ChartLayout — line-chart and bar-chart only use it as a local variable name. Simplify the visibility logic to only support waitForMeasurement and update tests accordingly. Co-Authored-By: Claude Opus 4.6 --- .../private/chart-layout/chart-layout.tsx | 10 ----- .../chart-layout/test/chart-layout.test.tsx | 39 ++----------------- 2 files changed, 3 insertions(+), 46 deletions(-) diff --git a/projects/js-packages/charts/src/charts/private/chart-layout/chart-layout.tsx b/projects/js-packages/charts/src/charts/private/chart-layout/chart-layout.tsx index af7d929f5d98..ad46bcec43a8 100644 --- a/projects/js-packages/charts/src/charts/private/chart-layout/chart-layout.tsx +++ b/projects/js-packages/charts/src/charts/private/chart-layout/chart-layout.tsx @@ -33,8 +33,6 @@ export interface ChartLayoutProps { trailingContent?: ReactNode; /** When true, hides the layout until content measurement is available */ waitForMeasurement?: boolean; - /** @deprecated Use waitForMeasurement instead */ - isWaitingForMeasurement?: boolean; /** Gap between Stack items */ gap?: GapSize; /** Additional class names */ @@ -56,7 +54,6 @@ export const ChartLayout = forwardRef< HTMLDivElement, ChartLayoutProps >( children, trailingContent, waitForMeasurement, - isWaitingForMeasurement, gap, className, style, @@ -68,17 +65,10 @@ export const ChartLayout = forwardRef< HTMLDivElement, ChartLayoutProps >( const [ contentRef, contentWidth, contentHeight ] = useElementSize< HTMLDivElement >(); const isMeasured = contentHeight > 0; - // Determine visibility: new waitForMeasurement prop takes precedence over legacy isWaitingForMeasurement let visibilityStyle: { visibility?: 'hidden' | 'visible' } = {}; if ( waitForMeasurement !== undefined ) { - // New API: hide until measured const isHidden = waitForMeasurement && ! isMeasured; visibilityStyle = { visibility: isHidden ? 'hidden' : 'visible' }; - } else if ( isWaitingForMeasurement !== undefined ) { - // Legacy API - visibilityStyle = { - visibility: isWaitingForMeasurement ? 'hidden' : 'visible', - }; } const isRenderProp = typeof children === 'function'; diff --git a/projects/js-packages/charts/src/charts/private/chart-layout/test/chart-layout.test.tsx b/projects/js-packages/charts/src/charts/private/chart-layout/test/chart-layout.test.tsx index 0c8fd8bf92da..bbd91806cc41 100644 --- a/projects/js-packages/charts/src/charts/private/chart-layout/test/chart-layout.test.tsx +++ b/projects/js-packages/charts/src/charts/private/chart-layout/test/chart-layout.test.tsx @@ -68,12 +68,12 @@ describe( 'ChartLayout', () => { expect( mockRenderLegendSlot ).toHaveBeenCalledWith( legendChildren, 'bottom' ); } ); - it( 'applies visibility hidden when isWaitingForMeasurement is true', () => { + it( 'applies visibility hidden when waitForMeasurement is true and not yet measured', () => { render(
Chart
@@ -82,27 +82,12 @@ describe( 'ChartLayout', () => { expect( screen.getByTestId( 'layout' ) ).toHaveStyle( { visibility: 'hidden' } ); } ); - it( 'applies visibility visible when isWaitingForMeasurement is false', () => { - render( - -
Chart
- - ); - expect( screen.getByTestId( 'layout' ) ).toHaveStyle( { visibility: 'visible' } ); - } ); - - it( 'does not set visibility in inline style when isWaitingForMeasurement is undefined', () => { + it( 'does not set visibility when waitForMeasurement is not provided', () => { render(
Chart
 ); - // When isWaitingForMeasurement is not provided, visibility should not be set in inline style const layoutStyle = screen.getByTestId( 'layout' ).getAttribute( 'style' ) ?? ''; expect( layoutStyle ).not.toContain( 'visibility' ); } ); @@ -165,24 +150,6 @@ describe( 'ChartLayout', () => { ); } ); - it( 'gives waitForMeasurement precedence over isWaitingForMeasurement', () => { - render( - -
Chart
- - ); - - // waitForMeasurement=true with no measurement yet should hide, - // even though isWaitingForMeasurement=false would show - expect( screen.getByTestId( 'layout' ) ).toHaveStyle( { visibility: 'hidden' } ); - } ); - it( 'renders trailing content after bottom legend', () => { render( Date: Thu, 12 Mar 2026 20:53:18 +1300 Subject: [PATCH 24/37] fix(charts): Use useEffect callback instead of setState during render Calling setMeasuredChartHeight inside ChartLayout's render prop triggered a React warning: "Cannot update a component while rendering a different component". Add an onContentHeightChange callback prop to ChartLayout that fires via useEffect after measurement, avoiding the cross-component state update during render. Co-Authored-By: Claude Opus 4.6 --- .../src/charts/line-chart/line-chart.tsx | 26 ++++++++++++++----- .../private/chart-layout/chart-layout.tsx | 11 +++++++- 2 files changed, 30 insertions(+), 7 deletions(-) diff --git a/projects/js-packages/charts/src/charts/line-chart/line-chart.tsx b/projects/js-packages/charts/src/charts/line-chart/line-chart.tsx index fec42ba1285b..afae1c0bd489 100644 --- a/projects/js-packages/charts/src/charts/line-chart/line-chart.tsx +++ b/projects/js-packages/charts/src/charts/line-chart/line-chart.tsx @@ -6,7 +6,15 @@ import { XYChart, AreaSeries, Grid, Axis, DataContext } from '@visx/xychart'; import { __ } from '@wordpress/i18n'; import clsx from 'clsx'; import { differenceInHours, differenceInYears } from 'date-fns'; -import { useMemo, useContext, forwardRef, useImperativeHandle, useState, useRef } from 'react'; +import { + useMemo, + useContext, + forwardRef, + useImperativeHandle, + useState, + useRef, + useCallback, +} from 'react'; import { Legend, useChartLegendItems } from '../../components/legend'; import { AccessibleTooltip, useKeyboardNavigation } from '../../components/tooltip'; import { @@ -296,6 +304,16 @@ const LineChartInternal = forwardRef< SingleChartRef, LineChartProps >( const hasLegend = showLegend || hasLegendChild; const [ measuredChartHeight, setMeasuredChartHeight ] = useState< number | undefined >(); + // Callback for ChartLayout to notify us when the measured content height changes. + // We compute chartHeight the same way the render prop does so the context stays in sync. + const handleContentHeightChange = useCallback( + ( contentHeight: number ) => { + const chartHeight = contentHeight > 0 ? contentHeight : height; + setMeasuredChartHeight( chartHeight ); + }, + [ height ] + ); + // Forward the external ref to the internal ref useImperativeHandle( ref, @@ -483,17 +501,13 @@ const LineChartInternal = forwardRef< SingleChartRef, LineChartProps >( style={ { width, height } } data-testid="line-chart" trailingContent={ nonLegendChildren } + onContentHeightChange={ handleContentHeightChange } > { ( { contentHeight } ) => { // Use the measured height, falling back to the passed height if provided. const chartHeight = contentHeight > 0 ? contentHeight : height; const isWaitingForMeasurement = hasLegend ? contentHeight === 0 : ! chartHeight; - // Update the measured chart height for context consumers (e.g. AnnotationsOverlay) - if ( chartHeight !== measuredChartHeight ) { - setMeasuredChartHeight( chartHeight ); - } - return (
void; /** Gap between Stack items */ gap?: GapSize; /** Additional class names */ @@ -54,6 +56,7 @@ export const ChartLayout = forwardRef< HTMLDivElement, ChartLayoutProps >( children, trailingContent, waitForMeasurement, + onContentHeightChange, gap, className, style, @@ -71,6 +74,12 @@ export const ChartLayout = forwardRef< HTMLDivElement, ChartLayoutProps >( visibilityStyle = { visibility: isHidden ? 'hidden' : 'visible' }; } + useEffect( () => { + if ( onContentHeightChange ) { + onContentHeightChange( contentHeight ); + } + }, [ contentHeight, onContentHeightChange ] ); + const isRenderProp = typeof children === 'function'; const renderedChildren = isRenderProp ? children( { contentWidth, contentHeight, isMeasured } ) From f2a538b76225bd1bf6f3c9ee2b449712074d4619 Mon Sep 17 00:00:00 2001 From: Adam Wood <1017872+adamwoodnz@users.noreply.github.com> Date: Fri, 13 Mar 2026 11:15:10 +1300 Subject: [PATCH 25/37] docs(charts): Document waitForMeasurement render-prop requirement waitForMeasurement only works with render-prop children because contentRef is only attached in that code path. With plain ReactNode children, isMeasured stays false and the layout remains permanently hidden. Add JSDoc warning to prevent silent misuse. Co-Authored-By: Claude Opus 4.6 --- .../charts/src/charts/private/chart-layout/chart-layout.tsx | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/projects/js-packages/charts/src/charts/private/chart-layout/chart-layout.tsx b/projects/js-packages/charts/src/charts/private/chart-layout/chart-layout.tsx index b26df03cfa91..1e5fce47dfac 100644 --- a/projects/js-packages/charts/src/charts/private/chart-layout/chart-layout.tsx +++ b/projects/js-packages/charts/src/charts/private/chart-layout/chart-layout.tsx @@ -31,7 +31,11 @@ export interface ChartLayoutProps { children: ReactNode | ( ( measurements: ContentMeasurements ) => ReactNode ); /** Content rendered after the bottom legend (e.g., nonLegendChildren, htmlChildren, tooltips) */ trailingContent?: ReactNode; - /** When true, hides the layout until content measurement is available */ + /** + * When true, hides the layout until content measurement is available. + * Only works with render-prop children — contentRef is not attached for plain ReactNode children, + * so isMeasured would stay false and the layout would remain permanently hidden. + */ waitForMeasurement?: boolean; /** Called when the measured content height changes (for render-prop mode) */ onContentHeightChange?: ( height: number ) => void; From fb0cb6e8c1a5ca139a214d5ad8eadccd1cc54883 Mon Sep 17 00:00:00 2001 From: Adam Wood <1017872+adamwoodnz@users.noreply.github.com> Date: Fri, 13 Mar 2026 11:25:58 +1300 Subject: [PATCH 26/37] refactor(charts): Remove unused chartWidth/chartHeight from pie and leaderboard context Pie, semi-circle, and leaderboard charts don't have consumers that read chartWidth/chartHeight from SingleChartContext. Passing zeros was misleading. Make the fields optional on ChartInstanceContextValue and omit them from charts that don't provide meaningful values. Co-Authored-By: Claude Opus 4.6 --- .../charts/src/charts/leaderboard-chart/leaderboard-chart.tsx | 4 ++-- .../js-packages/charts/src/charts/pie-chart/pie-chart.tsx | 2 +- .../charts/pie-semi-circle-chart/pie-semi-circle-chart.tsx | 2 +- .../private/single-chart-context/single-chart-context.tsx | 4 ++-- 4 files changed, 6 insertions(+), 6 deletions(-) diff --git a/projects/js-packages/charts/src/charts/leaderboard-chart/leaderboard-chart.tsx b/projects/js-packages/charts/src/charts/leaderboard-chart/leaderboard-chart.tsx index 06ae954ca4de..d17b0b88aaee 100644 --- a/projects/js-packages/charts/src/charts/leaderboard-chart/leaderboard-chart.tsx +++ b/projects/js-packages/charts/src/charts/leaderboard-chart/leaderboard-chart.tsx @@ -247,7 +247,7 @@ const LeaderboardChartInternal: FC< LeaderboardChartProps > = ( { // Handle empty or undefined data if ( ! data || data.length === 0 ) { return ( - + = ( { ); return ( - + + = ( { ); return ( - + ; - chartWidth: number; - chartHeight: number; + chartWidth?: number; + chartHeight?: number; } export const ChartInstanceContext = createContext< ChartInstanceContextValue | null >( null ); From 11ce567386758841913898728d00303261d1fc3d Mon Sep 17 00:00:00 2001 From: Adam Wood <1017872+adamwoodnz@users.noreply.github.com> Date: Fri, 13 Mar 2026 11:26:30 +1300 Subject: [PATCH 27/37] fix(charts): Pass measured chart height to SingleChartContext in BarChart Same pattern as LineChart: track the measured content height via onContentHeightChange callback so SingleChartContext.chartHeight reflects the actual drawable area, not the full container height. Co-Authored-By: Claude Opus 4.6 --- .../charts/src/charts/bar-chart/bar-chart.tsx | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/projects/js-packages/charts/src/charts/bar-chart/bar-chart.tsx b/projects/js-packages/charts/src/charts/bar-chart/bar-chart.tsx index 47734e89ac63..3123d23da938 100644 --- a/projects/js-packages/charts/src/charts/bar-chart/bar-chart.tsx +++ b/projects/js-packages/charts/src/charts/bar-chart/bar-chart.tsx @@ -118,6 +118,15 @@ const BarChartInternal: FC< BarChartProps > = ( { const { legendChildren, nonLegendChildren } = useChartChildren( children, 'BarChart' ); const hasLegendChild = legendChildren.length > 0; const hasLegend = showLegend || hasLegendChild; + const [ measuredChartHeight, setMeasuredChartHeight ] = useState< number | undefined >(); + + const handleContentHeightChange = useCallback( + ( contentHeight: number ) => { + const chartHeight = contentHeight > 0 ? contentHeight : height; + setMeasuredChartHeight( chartHeight ); + }, + [ height ] + ); const [ selectedIndex, setSelectedIndex ] = useState< number | undefined >( undefined ); const [ isNavigating, setIsNavigating ] = useState( false ); @@ -335,7 +344,7 @@ const BarChartInternal: FC< BarChartProps > = ( { value={ { chartId, chartWidth: width, - chartHeight: height || 0, + chartHeight: measuredChartHeight || 0, } } > = ( { data-testid="bar-chart" data-chart-id={ `bar-chart-${ chartId }` } trailingContent={ nonLegendChildren } + onContentHeightChange={ handleContentHeightChange } > { ( { contentHeight } ) => { const chartHeight = contentHeight > 0 ? contentHeight : height; From e9cc9a356586c75b771f0746b731ee8354e2be39 Mon Sep 17 00:00:00 2001 From: Adam Wood <1017872+adamwoodnz@users.noreply.github.com> Date: Fri, 13 Mar 2026 11:32:59 +1300 Subject: [PATCH 28/37] refactor(charts): Auto-hide ChartLayout during measurement for render-prop children Replace the opt-in waitForMeasurement prop with automatic behavior: when children is a render prop, ChartLayout applies visibility:hidden until contentHeight > 0. This prevents layout shift consistently across all charts without each chart needing to opt in. Plain ReactNode children are unaffected since they don't depend on measured dimensions. Co-Authored-By: Claude Opus 4.6 --- .../private/chart-layout/chart-layout.tsx | 20 ++++++------------- .../chart-layout/test/chart-layout.test.tsx | 15 ++++++-------- 2 files changed, 12 insertions(+), 23 deletions(-) diff --git a/projects/js-packages/charts/src/charts/private/chart-layout/chart-layout.tsx b/projects/js-packages/charts/src/charts/private/chart-layout/chart-layout.tsx index 1e5fce47dfac..6d4195189a6b 100644 --- a/projects/js-packages/charts/src/charts/private/chart-layout/chart-layout.tsx +++ b/projects/js-packages/charts/src/charts/private/chart-layout/chart-layout.tsx @@ -31,12 +31,6 @@ export interface ChartLayoutProps { children: ReactNode | ( ( measurements: ContentMeasurements ) => ReactNode ); /** Content rendered after the bottom legend (e.g., nonLegendChildren, htmlChildren, tooltips) */ trailingContent?: ReactNode; - /** - * When true, hides the layout until content measurement is available. - * Only works with render-prop children — contentRef is not attached for plain ReactNode children, - * so isMeasured would stay false and the layout would remain permanently hidden. - */ - waitForMeasurement?: boolean; /** Called when the measured content height changes (for render-prop mode) */ onContentHeightChange?: ( height: number ) => void; /** Gap between Stack items */ @@ -59,7 +53,6 @@ export const ChartLayout = forwardRef< HTMLDivElement, ChartLayoutProps >( legendChildren, children, trailingContent, - waitForMeasurement, onContentHeightChange, gap, className, @@ -70,21 +63,20 @@ export const ChartLayout = forwardRef< HTMLDivElement, ChartLayoutProps >( ref ) => { const [ contentRef, contentWidth, contentHeight ] = useElementSize< HTMLDivElement >(); + const isRenderProp = typeof children === 'function'; const isMeasured = contentHeight > 0; - let visibilityStyle: { visibility?: 'hidden' | 'visible' } = {}; - if ( waitForMeasurement !== undefined ) { - const isHidden = waitForMeasurement && ! isMeasured; - visibilityStyle = { visibility: isHidden ? 'hidden' : 'visible' }; - } + // When using render-prop children, hide the layout until measurement is available + // to prevent layout shift. Plain ReactNode children don't need this since they + // don't depend on measured dimensions. + const visibilityStyle: { visibility?: 'hidden' | 'visible' } = + isRenderProp && ! isMeasured ? { visibility: 'hidden' } : {}; useEffect( () => { if ( onContentHeightChange ) { onContentHeightChange( contentHeight ); } }, [ contentHeight, onContentHeightChange ] ); - - const isRenderProp = typeof children === 'function'; const renderedChildren = isRenderProp ? children( { contentWidth, contentHeight, isMeasured } ) : children; diff --git a/projects/js-packages/charts/src/charts/private/chart-layout/test/chart-layout.test.tsx b/projects/js-packages/charts/src/charts/private/chart-layout/test/chart-layout.test.tsx index bbd91806cc41..691da1b482f0 100644 --- a/projects/js-packages/charts/src/charts/private/chart-layout/test/chart-layout.test.tsx +++ b/projects/js-packages/charts/src/charts/private/chart-layout/test/chart-layout.test.tsx @@ -68,21 +68,18 @@ describe( 'ChartLayout', () => { expect( mockRenderLegendSlot ).toHaveBeenCalledWith( legendChildren, 'bottom' ); } ); - it( 'applies visibility hidden when waitForMeasurement is true and not yet measured', () => { + it( 'hides layout until measured when using render-prop children', () => { + const childFn = jest.fn().mockReturnValue(
Chart
); render( - -
Chart
+ + { childFn } ); + // Before ResizeObserver fires, contentHeight is 0 so layout should be hidden expect( screen.getByTestId( 'layout' ) ).toHaveStyle( { visibility: 'hidden' } ); } ); - it( 'does not set visibility when waitForMeasurement is not provided', () => { + it( 'does not hide layout when using plain ReactNode children', () => { render(
Chart
From e242b7d1f5bfe82aee9a3d86d687072361f93e70 Mon Sep 17 00:00:00 2001 From: Adam Wood <1017872+adamwoodnz@users.noreply.github.com> Date: Fri, 13 Mar 2026 11:49:20 +1300 Subject: [PATCH 29/37] refactor(charts): Simplify measurement gating, mock element size in tests ChartLayout now auto-hides render-prop children until measured, so individual charts no longer need their own isWaitingForMeasurement logic. Simplify bar/line charts to gate on chartHeight > 0 directly and remove the now-unused hasLegend variables. Add a getBoundingClientRect + ResizeObserver mock to the test setup so useElementSize returns non-zero dimensions in JSDOM. Without this, the auto-hide visibility guard keeps charts permanently hidden in tests. Co-Authored-By: Claude Opus 4.6 --- .../charts/src/charts/bar-chart/bar-chart.tsx | 5 +-- .../src/charts/line-chart/line-chart.tsx | 6 +-- .../js-packages/charts/tests/jest.config.cjs | 1 + .../charts/tests/setup-element-size-mock.js | 42 +++++++++++++++++++ 4 files changed, 45 insertions(+), 9 deletions(-) create mode 100644 projects/js-packages/charts/tests/setup-element-size-mock.js diff --git a/projects/js-packages/charts/src/charts/bar-chart/bar-chart.tsx b/projects/js-packages/charts/src/charts/bar-chart/bar-chart.tsx index 3123d23da938..e8e46abae860 100644 --- a/projects/js-packages/charts/src/charts/bar-chart/bar-chart.tsx +++ b/projects/js-packages/charts/src/charts/bar-chart/bar-chart.tsx @@ -116,8 +116,6 @@ const BarChartInternal: FC< BarChartProps > = ( { // Process children for composition API (Legend, etc.) const { legendChildren, nonLegendChildren } = useChartChildren( children, 'BarChart' ); - const hasLegendChild = legendChildren.length > 0; - const hasLegend = showLegend || hasLegendChild; const [ measuredChartHeight, setMeasuredChartHeight ] = useState< number | undefined >(); const handleContentHeightChange = useCallback( @@ -369,7 +367,6 @@ const BarChartInternal: FC< BarChartProps > = ( { > { ( { contentHeight } ) => { const chartHeight = contentHeight > 0 ? contentHeight : height; - const isWaitingForMeasurement = hasLegend ? contentHeight === 0 : ! chartHeight; return (
= ( { onFocus={ onChartFocus } onBlur={ onChartBlur } > - { ! isWaitingForMeasurement && ( + { chartHeight > 0 && (
( // Process children for composition API (Legend, etc.) const { legendChildren, nonLegendChildren } = useChartChildren( children, 'LineChart' ); - const hasLegendChild = legendChildren.length > 0; - - const hasLegend = showLegend || hasLegendChild; const [ measuredChartHeight, setMeasuredChartHeight ] = useState< number | undefined >(); // Callback for ChartLayout to notify us when the measured content height changes. @@ -506,7 +503,6 @@ const LineChartInternal = forwardRef< SingleChartRef, LineChartProps >( { ( { contentHeight } ) => { // Use the measured height, falling back to the passed height if provided. const chartHeight = contentHeight > 0 ? contentHeight : height; - const isWaitingForMeasurement = hasLegend ? contentHeight === 0 : ! chartHeight; return (
( onFocus={ onChartFocus } onBlur={ onChartBlur } > - { ! isWaitingForMeasurement && ( + { chartHeight > 0 && (