Skip to content

Charts: Use Stack for layout #47981

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
adamwoodnz merged 14 commits into from
Apr 14, 2026
Merged

Charts: Use Stack for layout #47981

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
83b38b4
Charts: Migrate legend container to Stack from @wordpress/ui
adamwoodnz Apr 7, 2026
0036a59
Charts: Migrate conversion funnel layouts to Stack
adamwoodnz Apr 7, 2026
366c982
Charts: Migrate line-chart tooltip and popover layouts to Stack
adamwoodnz Apr 7, 2026
2fdfb42
Charts: Migrate geo-chart container to Stack
adamwoodnz Apr 7, 2026
2f9f2a6
Charts: Replace __experimentalHStack with Stack in donut story
adamwoodnz Apr 7, 2026
bbe65d8
Charts: Add changelog for CHARTS-175 Stack migration
adamwoodnz Apr 7, 2026
676037a
Charts: Remove unused className from annotation popover header Stack
cursoragent Apr 7, 2026
b0ff694
Charts: Migrate conversion funnel tooltip layout to Stack
adamwoodnz Apr 8, 2026
5227124
Charts: Migrate funnel bar-container to Stack
adamwoodnz Apr 8, 2026
7d30e8d
Charts: Lift funnel main-metric margin to outer Stack gap
adamwoodnz Apr 8, 2026
902ec03
Charts: Migrate legend item label layout to Stack
adamwoodnz Apr 8, 2026
f5db6a3
Charts: Drop redundant flex rules from .legend-item
adamwoodnz Apr 8, 2026
03ca39d
Charts: Hoist alignmentToFlex map to module scope in BaseLegend
cursoragent Apr 8, 2026
58b79c3
Charts: Fix regression in alignment of CustomPieLegend Stack
adamwoodnz Apr 8, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions projects/js-packages/charts/changelog/charts-175-use-stack-for-layout
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
Significance: patch
Type: changed

Charts: Replace ad-hoc flexbox layouts with @wordpress/ui Stack across legend, conversion funnel, line chart, geo chart, conversion funnel tooltip, and donut story.
21 changes: 0 additions & 21 deletions ...js-packages/charts/src/charts/conversion-funnel-chart/conversion-funnel-chart.module.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,6 @@
}

.main-metric {
display: flex;
align-items: baseline;
gap: 8px;
margin-bottom: 24px;
height: 20px;
}

Expand All @@ -36,9 +32,6 @@
}

.funnel-container {
display: flex;
gap: 16px;
align-items: flex-end;
flex: 1;
min-height: 200px;
width: 100%;
Expand All @@ -47,8 +40,6 @@
.funnel-step {
flex: 1 1 0;
min-width: 0;
display: flex;
flex-direction: column;
height: 100%;

&--animated {
Expand All @@ -60,10 +51,6 @@
}
}

.step-header {
margin-bottom: 24px;
}

.step-label {
color: #757575;
font-size: var(--wpds-font-size-sm, 12px);
Expand All @@ -88,8 +75,6 @@

.bar-container {
flex: 1;
display: flex;
align-items: flex-end;
border-radius: 4px;
position: relative;
cursor: pointer;
Expand All @@ -116,12 +101,6 @@
}

.tooltip-wrapper {
display: inline-flex;
flex-direction: column;
justify-content: center;
align-items: flex-start;
gap: 4px;

border-bottom: 1px solid var(--Gray-Gray-5, #dcdcde);
background: var(--black-white-white, #fff);

Expand Down
27 changes: 17 additions & 10 deletions projects/js-packages/charts/src/charts/conversion-funnel-chart/conversion-funnel-chart.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -263,13 +263,13 @@ const ConversionFunnelChartInternal: FC< ConversionFunnelChartProps > = ( {

// Default tooltip rendering function
const renderDefaultTooltip = ( step: FunnelStep ) => (
<>
<Stack direction="column" align="flex-start" gap="xs">
Copy link
Copy Markdown
Contributor

@dognose24 dognose24 Apr 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Image

Works as previously.

<div className={ styles[ 'tooltip-title' ] }>{ step.label }</div>
<div className={ styles[ 'tooltip-content' ] }>
{ formatPercentage( step.rate ) }
{ ` • ${ step.count ?? 'no' } items` }
</div>
</>
</Stack>
);

// Validate data
Expand Down Expand Up @@ -322,6 +322,7 @@ const ConversionFunnelChartInternal: FC< ConversionFunnelChartProps > = ( {
<>
<Stack
direction="column"
gap="xl"
data-testid="conversion-funnel-chart"
ref={ node => {
// Set containerRef for @visx coordinate system
Expand All @@ -344,27 +345,31 @@ const ConversionFunnelChartInternal: FC< ConversionFunnelChartProps > = ( {
changeColor,
} )
) : (
<div className={ styles[ 'main-metric' ] }>{ renderDefaultMainMetric() }</div>
<Stack direction="row" align="baseline" gap="sm" className={ styles[ 'main-metric' ] }>
{ renderDefaultMainMetric() }
</Stack>
) }

{ /* Funnel Steps */ }
<div className={ styles[ 'funnel-container' ] }>
<Stack direction="row" align="flex-end" gap="lg" className={ styles[ 'funnel-container' ] }>
Copy link
Copy Markdown
Contributor

@dognose24 dognose24 Apr 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Image

{ steps.map( ( step, index ) => {
const barHeight = ( step.rate / maxRate ) * 100;
const { isBlurred } = getStepState( step.id );

return (
<div
<Stack
key={ step.id }
direction="column"
data-testid="funnel-step"
className={ clsx(
styles[ 'funnel-step' ],
isColorPaletteResolved && styles[ 'funnel-step--animated' ],
isBlurred && styles[ 'funnel-step--blurred' ]
) }
gap="xl"
>
{ /* Step Label and Rate */ }
<div className={ styles[ 'step-header' ] }>
<div>
{ renderStepLabel ? (
renderStepLabel( {
step,
Copy link
Copy Markdown
Contributor

@kangzj kangzj Apr 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Micro-cleanup suggestion: this <div> used to carry styles['step-header'] purely for the margin-bottom: 24px that's now handled by the parent Stack's gap="xl". With no className and no other purpose, it could probably collapse into a <Fragment> (or disappear entirely) to shave one DOM node. Very minor — keep it if you prefer the explicit grouping.

Copy link
Copy Markdown
Contributor

@dognose24 dognose24 Apr 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree with it! 👍🏼

Copy link
Copy Markdown
Contributor Author

@adamwoodnz adamwoodnz Apr 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately this won't work sorry; without the wrapper an xl sized gap will appear between the label and rate, due to the parent Stack:

Screenshot 2026-04-14 at 11 57 48 AM

They need to be wrapped to maintain their internal layout.

Expand All @@ -388,7 +393,9 @@ const ConversionFunnelChartInternal: FC< ConversionFunnelChartProps > = ( {
</div>

{ /* Funnel Bar */ }
<div
<Stack
direction="column"
justify="flex-end"
className={ styles[ 'bar-container' ] }
onClick={ stepHandlers.get( step.id )?.onClick }
onKeyDown={ stepHandlers.get( step.id )?.onKeyDown }
Expand All @@ -407,11 +414,11 @@ const ConversionFunnelChartInternal: FC< ConversionFunnelChartProps > = ( {
backgroundColor: barColor,
} }
/>
</div>
</div>
</Stack>
</Stack>
);
} ) }
</div>
</Stack>
</Stack>

{ /* Tooltip Portal */ }
Expand Down
3 changes: 0 additions & 3 deletions projects/js-packages/charts/src/charts/geo-chart/geo-chart.module.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,3 @@
.container {
position: relative;
display: flex;
justify-content: center;
align-items: center;
}
13 changes: 9 additions & 4 deletions projects/js-packages/charts/src/charts/geo-chart/geo-chart.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
* External dependencies
*/
import { __ } from '@wordpress/i18n';
import { Stack } from '@wordpress/ui';
import clsx from 'clsx';
import { FC, useContext, useMemo } from 'react';
import { Chart, type GoogleChartOptions } from 'react-google-charts';
Expand Down Expand Up @@ -57,13 +58,15 @@ const GeoChartInternal: FC< GeoChartProps > = ( {

// Render loading placeholder
const loadingPlaceholder = (
<div
<Stack
align="center"
justify="center"
className={ clsx( 'geo-chart', styles.container, className ) }
data-testid="geo-chart-loading"
style={ { width, height } }
>
Copy link
Copy Markdown
Contributor

@kangzj kangzj Apr 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both the loading placeholder and the main container use <Stack align="center" justify="center"> around a single child — so Stack here is really acting as a centered flex wrapper rather than a layout primitive. Not wrong at all, just worth noting for consistency: a plain <div> with centering CSS (or a dedicated centering primitive if one exists) might be a more honest signal of intent. Happy to defer to whatever keeps the package consistent.

Copy link
Copy Markdown
Contributor Author

@adamwoodnz adamwoodnz Apr 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I think this is still a valid use of a Stack despite the name. I think a more appropriate name for the Stack component might be Flex, as that's what it provides essentially. That said, a centering component abstraction could be useful, I'll take a look at what sort of usage this would have.

Copy link
Copy Markdown
Contributor Author

@adamwoodnz adamwoodnz Apr 14, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Claude's take: a thin <Center> primitive is warranted, but only barely. Five sites is around the threshold where an abstraction starts paying for itself, and two of them already hand-named the centering intent via *__centering CSS classes — that's a tell that "centered box" is a recurring concept, not incidental flexing. A

component would:

  • Communicate intent (kangzj's point — Stack-as-centering reads awkwardly)
  • Let the pie charts drop their *__centering class names
  • Be ~10 lines wrapping Stack with fixed align="center" justify="center" defaults

Tradeoff: one more primitive to learn, and the implementation is trivially a Stack underneath. If you're renaming Stack → Flex anyway (per your reply), consider doing both together — <Flex> for general flex layout, <Center> for the centering case. Separate follow-up PR, not this one.

My take: I didn't mean we'd actually rename Stack to Flex. I think a Center component does make sense given we have existing styles doing this. I'll create a separate issue.

Copy link
Copy Markdown
Contributor Author

@adamwoodnz adamwoodnz Apr 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CHARTS-212: Add a Center layout primitive

{ renderPlaceholder ? renderPlaceholder() : __( 'Loading map', 'jetpack-charts' ) }
</div>
</Stack>
);

// Google charts doesn't accept CSS variables, so we need to convert them to hex colors
Expand Down Expand Up @@ -144,7 +147,9 @@ const GeoChartInternal: FC< GeoChartProps > = ( {
);

return (
<div
<Stack
Copy link
Copy Markdown
Contributor

@dognose24 dognose24 Apr 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Image

align="center"
justify="center"
className={ clsx( 'geo-chart', styles.container, className ) }
data-testid="geo-chart"
style={ { width, height, backgroundColor } }
Expand All @@ -157,7 +162,7 @@ const GeoChartInternal: FC< GeoChartProps > = ( {
options={ options }
loader={ loadingPlaceholder }
/>
</div>
</Stack>
);
};

Expand Down
10 changes: 0 additions & 10 deletions projects/js-packages/charts/src/charts/line-chart/line-chart.module.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,10 +26,7 @@
}

&__tooltip-row {
display: flex;
align-items: center;
padding: 4px 0;
justify-content: space-between;
}

&__tooltip-label {
Expand Down Expand Up @@ -81,13 +78,6 @@
}
}

&__annotation-label-popover-header {
display: flex;
flex-direction: row;
justify-content: space-between;
align-items: start;
}

&__annotation-label-popover-content {
padding: 0.5rem;
}
Expand Down
11 changes: 9 additions & 2 deletions projects/js-packages/charts/src/charts/line-chart/line-chart.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ 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 {
Expand Down Expand Up @@ -103,12 +104,18 @@ const renderDefaultTooltip = ( params: RenderTooltipParams< DataPointDate > ) =>
{ nearestDatum.date?.toLocaleDateString() }
</div>
{ tooltipPoints.map( point => (
<div key={ point.key } className={ styles[ 'line-chart__tooltip-row' ] }>
<Stack
Copy link
Copy Markdown
Contributor

@dognose24 dognose24 Apr 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Image

key={ point.key }
direction="row"
align="center"
justify="space-between"
className={ styles[ 'line-chart__tooltip-row' ] }
>
<span className={ styles[ 'line-chart__tooltip-label' ] }>{ point.key }:</span>
<span className={ styles[ 'line-chart__tooltip-value' ] }>
{ formatNumber( point.value ) }
</span>
</div>
</Stack>
) ) }
</div>
);
Expand Down
5 changes: 3 additions & 2 deletions .../js-packages/charts/src/charts/line-chart/private/line-chart-annotation-label-popover.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import { __ } from '@wordpress/i18n';
import { Icon, close } from '@wordpress/icons';
import { Stack } from '@wordpress/ui';
import clsx from 'clsx';
import { useEffect, useId, useRef, useState } from 'react';
import { isSafari } from '../../../utils';
Expand Down Expand Up @@ -88,7 +89,7 @@ const LineChartAnnotationLabelWithPopover: FC< LineChartAnnotationLabelWithPopov
) }
data-testid="line-chart-annotation-label-popover"
>
<div className={ styles[ 'line-chart__annotation-label-popover-header' ] }>
<Stack direction="row" align="flex-start" justify="space-between">
Copy link
Copy Markdown
Contributor

@dognose24 dognose24 Apr 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Image

<div className={ styles[ 'line-chart__annotation-label-popover-content' ] }>
{ renderLabelPopover( { title, subtitle } ) }
</div>
Expand All @@ -102,7 +103,7 @@ const LineChartAnnotationLabelWithPopover: FC< LineChartAnnotationLabelWithPopov
>
<Icon icon={ close } size={ 16 } />
</button>
</div>
</Stack>
</div>
</div>
);
Expand Down
8 changes: 3 additions & 5 deletions projects/js-packages/charts/src/charts/pie-chart/stories/donut.stories.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,4 @@
/* eslint-disable @wordpress/no-unsafe-wp-apis */
import { __experimentalHStack as HStack } from '@wordpress/components';
import { Text } from '@wordpress/ui';
import { Stack, Text } from '@wordpress/ui';
import { Fragment } from 'react';
import { BaseLegendItem } from '../../../components/legend/types';
import {
Expand Down Expand Up @@ -267,7 +265,7 @@ const CustomPieLegend = ( {

return (
<Fragment key={ index }>
<HStack direction="row" justify="flex-start" spacing={ 2 }>
<Stack direction="row" justify="flex-start" align="center" gap="sm">
<div
style={ {
width: '8px',
Expand All @@ -278,7 +276,7 @@ const CustomPieLegend = ( {
} }
/>
<Text variant="body-sm">{ item.label }</Text>
</HStack>
</Stack>
<Text variant="body-sm" style={ { fontWeight: 600, textAlign: 'right' } }>
{ item.formattedValue }
</Text>
Expand Down
49 changes: 0 additions & 49 deletions projects/js-packages/charts/src/components/legend/private/base-legend.module.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,49 +1,8 @@
.legend {
align-self: stretch;

&--horizontal {
display: flex;
flex-direction: row;
flex-wrap: wrap;
gap: 16px;
}

&--vertical {
display: flex;
flex-direction: column;
gap: 8px;
}

&--alignment-start {
justify-content: flex-start;
}

&--alignment-center {
justify-content: center;
}

&--alignment-end {
justify-content: flex-end;
}

// Vertical legends align on the cross-axis instead
&--vertical.legend--alignment-start {
align-items: flex-start;
}

&--vertical.legend--alignment-center {
align-items: center;
}

&--vertical.legend--alignment-end {
align-items: flex-end;
}

}

.legend-item {
display: flex;
align-items: center;
font-size: var(--wpds-font-size-md, 13px);

&--interactive {
Expand Down Expand Up @@ -75,14 +34,6 @@
}
}

.legend-item-label {
display: flex;
align-items: center;
gap: 0.5rem;

/* Text wrapping is handled at the text level, not the label container */
}

.legend-item-text {

&--wrap {
Expand Down
Loading
Loading