Charts: Use Stack for layout by adamwoodnz · Pull Request #47981 · Automattic/jetpack

adamwoodnz · 2026-04-07T20:54:44Z

Fixes CHARTS-175

Proposed changes

Migrate the legend container from a <div> with BEM modifier classes (.legend--horizontal, .legend--vertical, .legend--alignment-*) to a <Stack> from @wordpress/ui. The root .legend class is preserved as an identifying selector hook; layout-only modifier classes are dropped.
Migrate the legend item label inner layout (LegendText + value span) to a <Stack gap="sm">, replacing the display: flex; align-items: center; gap: 0.5rem SCSS rule. Drop the redundant display: flex; align-items: center rules from .legend-item — visx's LegendItem applies those as inline styles by default, so the SCSS was dead code.
Migrate conversion funnel layout wrappers (.main-metric, .funnel-container, .funnel-step, .bar-container) to <Stack>. The .bar-container keeps its button semantics (onClick/onKeyDown/role/tabIndex/aria-label) on the Stack wrapper.
Lift two ad-hoc margin-as-gap rules onto parent Stacks: .main-metric { margin-bottom: 24px } becomes gap="xl" on the outer funnel Stack, and .step-header { margin-bottom: 24px } (deleted entirely) becomes gap="xl" on the funnel-step Stack.
Migrate the conversion funnel default tooltip to <Stack>. The .tooltip-wrapper class now carries only visual chrome (border, padding, box-shadow); its layout properties move onto a <Stack> wrapping the tooltip children inside TooltipInPortal. Custom renderTooltip callers are responsible for their own layout.
Migrate line-chart tooltip row and annotation popover header to <Stack>. Button-internal icon centering (.annotation-label-trigger-button, .annotation-label-popover-close-button) stays as SCSS — it's single-icon button centering, not layout.
Migrate the geo-chart container (both the loading placeholder and the main render) to <Stack> with centered alignment. position: relative stays in SCSS because Google Charts' GeoChart relies on it for tooltip positioning.
Replace __experimentalHStack from @wordpress/components with Stack from @wordpress/ui in the donut story's CustomPieLegend example.
Gap values move from hardcoded px to design-system tokens: 4px → xs, 8px → sm, 16px → lg, 24px → xl. This contributes to CHARTS-199 (spacing/border tokenization).

Other information

Two deviations from the original issue scope, both documented in the commit messages:

Trend-indicator skipped. TrendIndicator is a publicly exported component rendered as <span display: inline-flex>. Stack from @wordpress/ui only renders <div>, which would change the element from inline to block — a backward-compat regression risk for consumers using it in flowing text. A follow-up is trivial if/when Stack gains an as prop.
One 2px sibling margin left in place. .step-label { margin: 0 0 2px 0 } couldn't be lifted to a Stack gap because @wordpress/ui Stack only accepts design-system tokens (xs=4px is the smallest), not raw pixel values. Migrating would have rounded 2px → 4px, so the margin stays as-is.

Related product discussion/links

CHARTS-175: Use Stack for layout
CHARTS-199: Spacing/border tokenization (this PR contributes)
pgvZfX-yD-p2

Does this pull request change what data or activity we track or use?

No.

Testing instructions

This is a refactor with no intended behavior changes. Verification is existing tests + visual parity in Storybook.

From the monorepo root: pnpm --filter @automattic/charts test — expect 841 passing tests across 43 suites.
From the monorepo root: pnpm --filter @automattic/charts typecheck — expect clean.
Start Storybook: cd projects/js-packages/storybook && pnpm run storybook:dev and navigate to each migrated surface. Layout should be visually identical to trunk:
- Legend: JS Packages / Charts Library / Components / Legend → Default (horizontal, items side-by-side with 16px gap) and Vertical (items stacked with 8px gap, center-aligned). Items with values should show 8px gap between label text and value.
- Conversion funnel: JS Packages / Charts Library / Charts / Conversion Funnel Chart → Default. Main metric (10.3% +2%) should be baseline-aligned with 8px between values, and 24px below the funnel. Four steps (Sessions/Cart/Checkout/Purchase) should have 16px gap, bars bottom-aligned, and 24px between each step's label/rate header and its bar. Click a funnel bar — the default tooltip should show title + rate/items with 4px gap between them.
- Line chart tooltip: JS Packages / Charts Library / Charts / Line Chart → Default. Hover the chart — tooltip rows should have label left, value right, space-between layout.
- Line chart annotations: JS Packages / Charts Library / Charts / Line Chart / Annotations → Default. Three annotation labels should render with title/subtitle column layout.
- Geo chart: JS Packages / Charts Library / Charts / Geo Chart → Default. World map should be centered in the container; tooltips on country hover should still position correctly.
- Donut custom legend: JS Packages / Charts Library / Charts / Donut Chart → Custom Legend. Legend rows should show color dot + label (8px gap) + right-aligned values and comparison percentages.

Note

This unrelated issue was discovered while testing: CHARTS-208

github-actions · 2026-04-07T20:55:36Z

Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.

To test on WoA, go to the Plugins menu on a WoA dev site. Click on the "Upload" button and follow the upgrade flow to be able to upload, install, and activate the Jetpack Beta plugin. Once the plugin is active, go to Jetpack > Jetpack Beta, select your plugin (Jetpack), and enable the charts-175-use-stack-for-layout branch.
To test on Simple, run the following command on your sandbox:

bin/jetpack-downloader test jetpack charts-175-use-stack-for-layout

Interested in more tips and information?

In your local development environment, use the jetpack rsync command to sync your changes to a WoA dev blog.
Read more about our development workflow here: PCYsg-eg0-p2
Figure out when your changes will be shipped to customers here: PCYsg-eg5-p2

github-actions · 2026-04-07T20:55:55Z

Thank you for your PR!

When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:

✅ Include a description of your PR changes.
✅ Add a "[Status]" label (In Progress, Needs Review, ...).
✅ Add testing instructions.
✅ Specify whether this PR includes any changes to data or privacy.
✅ Add changelog entries to affected projects

This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖

Follow this PR Review Process:

Ensure all required checks appearing at the bottom of this PR are passing.
Make sure to test your changes on all platforms that it applies to. You're responsible for the quality of the code you ship.
You can use GitHub's Reviewers functionality to request a review.
When it's reviewed and merged, you will be pinged in Slack to deploy the changes to WordPress.com simple once the build is done.

If you have questions about anything, reach out in #jetpack-developers for guidance!

jp-launch-control · 2026-04-07T21:09:49Z

Code Coverage Summary

Coverage changed in 1 file.

File	Coverage	Δ%	Δ Uncovered
projects/js-packages/charts/src/components/legend/private/base-legend.tsx	43/43 (100.00%)	0.00%	0 💚

Full summary · PHP report · JS report

...cts/js-packages/charts/src/charts/line-chart/private/line-chart-annotation-label-popover.tsx

Copilot

Pull request overview

Refactors the Charts package’s internal layout wrappers to use Stack from @wordpress/ui, replacing several ad-hoc flexbox <div> containers and removing now-redundant SCSS layout modifiers, while keeping visual layout behavior consistent and aligning spacing with design-system tokens.

Changes:

Migrated legend container layout from BEM-based flexbox classes to @wordpress/ui Stack (keeping the root .legend selector hook).
Migrated key chart layouts to Stack (conversion funnel wrappers, line-chart tooltip rows, annotation popover header, geo-chart container/placeholder).
Updated the donut chart Storybook custom legend example to use Stack instead of __experimentalHStack, and added a changelog fragment.

Reviewed changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
projects/js-packages/charts/src/components/legend/private/base-legend.tsx	Replaces legend container `<div>` flex styling with `Stack` props (direction/gap/alignment/wrap).
projects/js-packages/charts/src/components/legend/private/base-legend.module.scss	Removes legend container layout modifier rules now handled by `Stack`.
projects/js-packages/charts/src/charts/pie-chart/stories/donut.stories.tsx	Replaces `__experimentalHStack` usage with `Stack` in the custom legend story.
projects/js-packages/charts/src/charts/line-chart/private/line-chart-annotation-label-popover.tsx	Converts annotation popover header layout wrapper to `Stack`.
projects/js-packages/charts/src/charts/line-chart/line-chart.tsx	Converts tooltip row wrapper to `Stack` for flex layout.
projects/js-packages/charts/src/charts/line-chart/line-chart.module.scss	Removes flex layout rules now provided by `Stack` in tooltip rows and popover header.
projects/js-packages/charts/src/charts/geo-chart/geo-chart.tsx	Centers loading and main geo chart render using `Stack` instead of flexbox CSS.
projects/js-packages/charts/src/charts/geo-chart/geo-chart.module.scss	Removes flex centering styles while preserving `position: relative`.
projects/js-packages/charts/src/charts/conversion-funnel-chart/conversion-funnel-chart.tsx	Migrates main metric, funnel container, and step wrappers to `Stack` with tokenized gaps.
projects/js-packages/charts/src/charts/conversion-funnel-chart/conversion-funnel-chart.module.scss	Removes flex layout styles now handled by `Stack`.
projects/js-packages/charts/changelog/charts-175-use-stack-for-layout	Adds changelog fragment documenting the refactor.

adamwoodnz · 2026-04-08T03:02:05Z

@cursoragent code review please

cursor · 2026-04-08T03:02:14Z

Here's the code review for the PR, Adam. I found one thing worth fixing and went ahead and addressed it. The rest looks solid.

Issue fixed (committed):

alignmentToFlex was recreated on every render in base-legend.tsx. The mapping object ({ start: 'flex-start', center: 'center', end: 'flex-end' }) is static and doesn't depend on props or state, so I hoisted it to module scope as ALIGNMENT_TO_FLEX. This avoids an unnecessary object allocation per render.

Everything else looks good:

Token mapping (xs/sm/lg/xl) is correct against the @wordpress/theme GapSize type ('xs' | 'sm' | 'md' | 'lg' | 'xl' | '2xl' | '3xl'), and the values match the intent: xs=4px, sm=8px, lg=16px, xl=24px.
Stack prop usage is consistent with its typed API (direction, gap, align, justify, wrap all accept correct value types). The wrap prop correctly uses undefined for vertical and 'wrap' for horizontal.
SCSS cleanup is thorough: every removed display: flex/gap/align-items/justify-content/flex-direction rule has a corresponding Stack prop taking over. The deleted &__annotation-label-popover-header block (from the previous commit) was correctly dead code.
The .legend-item SCSS still declares display: flex; align-items: center (lines 6-7), which overlaps with visx LegendItem's inline styles. The PR description mentions dropping these as dead code, but they're still in the SCSS. They're harmless (visx inline styles win), so not blocking -- just noting the description/code mismatch.
Backward compat is preserved: no public API or element type changes. The ref forwarding on BaseLegend's Stack is correctly maintained. The funnel bar-container keeps its interactive attributes (onClick, onKeyDown, role, tabIndex, aria-label) as a plain div rather than moving them onto a Stack, which is the right call since it has semantic button behavior.
The donut story migration from __experimentalHStack to Stack is clean; the eslint-disable for @wordpress/no-unsafe-wp-apis is correctly removed since the experimental import is gone.
Changelog entry is well-formed.

adamwoodnz · 2026-04-08T03:12:48Z

The .legend-item SCSS still declares display: flex; align-items: center (lines 6-7), which overlaps with visx LegendItem's inline styles. The PR description mentions dropping these as dead code, but they're still in the SCSS. They're harmless (visx inline styles win), so not blocking -- just noting the description/code mismatch.

@cursoragent these are gone, I think the branch may have been slightly out of date

Replaces the legend container <div> + ad-hoc flex SCSS modifier classes with a <Stack> from @wordpress/ui. Gap values move from hardcoded px (8px/16px) to design-system tokens (sm/lg). Root .legend class is preserved as the component's identifying selector hook. Refs CHARTS-175.

Replaces .main-metric, .funnel-container, and .funnel-step ad-hoc flex divs with <Stack> from @wordpress/ui. Gap values move to design-system tokens (sm=8px, lg=16px). The .tooltip-wrapper stays as-is because it is rendered inside @visx/tooltip's TooltipInPortal, which controls its own DOM and positioning. Refs CHARTS-175.

Replaces the line-chart tooltip row and annotation popover header divs with <Stack> from @wordpress/ui. The trigger-button centering stays as SCSS — it's button-internal positioning, not layout. Refs CHARTS-175.

Both the loading placeholder and the main chart container are now <Stack> elements with centered alignment. position: relative stays in SCSS because Google Charts' GeoChart needs it for tooltip positioning. Refs CHARTS-175.

Migrates the CustomPieLegend example off @wordpress/components' experimental HStack onto the stable Stack from @wordpress/ui. Refs CHARTS-175.

Refs CHARTS-175.

The line-chart__annotation-label-popover-header class had no corresponding SCSS rule and was left over from the migration to Stack. Drop it.

The .tooltip-wrapper class was carrying both layout (inline-flex, column, gap: 4px) and visual chrome (border, padding, box-shadow), and it's applied to visx's TooltipInPortal wrapper. Previously I skipped this in 8501724 because the portal controls its own DOM, but the children inside it are ours — so wrap the default tooltip children in a <Stack> and keep only the visual styling on the portal wrapper. Custom renderTooltip callers are responsible for their own layout; the className they receive now provides only visual chrome, which matches the intent of a "custom" tooltip override. Refs CHARTS-175.

Replace the `.bar-container` button div with a `<Stack>` carrying the existing button semantics (onClick/onKeyDown/role/tabIndex/ aria-label). The remaining `.bar-container` SCSS keeps `flex: 1`, border-radius, position, and cursor — visual styling that doesn't fit Stack props. Also drop `.step-header` and lift its 24px bottom margin onto the parent funnel-step Stack as `gap="xl"`, which is the layout-token equivalent and removes a CSS rule we no longer need. Refs CHARTS-175.

The .main-metric class carried a margin-bottom: 24px to space it from the .funnel-container below. Now that both are siblings inside the outer column Stack, that 24px belongs on the parent as gap="xl" — the design-system token equivalent — so drop the SCSS margin and add the gap prop. Refs CHARTS-175.

The .legend-item-label SCSS rule carried display: flex, align-items: center, and gap: 0.5rem to lay out the label text + optional value span inside visx's LegendLabel. Wrap those children in a <Stack> with gap="sm" (8px) and drop the SCSS rule. The .legend-item-label class stays on LegendLabel as a selector hook for the .legend-item--inactive .legend-item-label line-through rule. The labelJustifyContent prop moves from LegendLabel's inline style onto the Stack, since the Stack is now the actual flex container. Refs CHARTS-175.

visx's LegendItem applies display: flex, align-items: center, flex-direction, and margin as inline styles by default (see @visx/legend's LegendItem.js), so the matching SCSS rules in .legend-item were dead code overridden by the inline styles. Delete them. Refs CHARTS-175.

The mapping is static and doesn't depend on props or state. Moving it out of the render body avoids recreating the object on every render.

github-actions bot added [JS Package] Charts RNA labels Apr 7, 2026

github-actions bot added the [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. label Apr 7, 2026

adamwoodnz added [Status] In Progress and removed [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. labels Apr 7, 2026

adamwoodnz self-assigned this Apr 7, 2026

adamwoodnz force-pushed the charts-175-use-stack-for-layout branch from 3bd6464 to ea3cef2 Compare April 7, 2026 21:59

adamwoodnz commented Apr 7, 2026

View reviewed changes

...cts/js-packages/charts/src/charts/line-chart/private/line-chart-annotation-label-popover.tsx Outdated Show resolved Hide resolved

adamwoodnz requested a review from Copilot April 8, 2026 00:08

Copilot started reviewing on behalf of adamwoodnz April 8, 2026 00:09 View session

Copilot AI reviewed Apr 8, 2026

View reviewed changes

adamwoodnz added [Status] Needs Review This PR is ready for review. and removed [Status] In Progress labels Apr 8, 2026

adamwoodnz force-pushed the charts-175-use-stack-for-layout branch from 404ea39 to 30e86ee Compare April 8, 2026 20:03

adamwoodnz requested a review from a team April 8, 2026 22:54

adamwoodnz force-pushed the charts-175-use-stack-for-layout branch from 30e86ee to 4ee6195 Compare April 9, 2026 02:18

adamwoodnz and others added 8 commits April 9, 2026 14:37

Charts: Migrate line-chart tooltip and popover layouts to Stack

0837622

Replaces the line-chart tooltip row and annotation popover header divs with <Stack> from @wordpress/ui. The trigger-button centering stays as SCSS — it's button-internal positioning, not layout. Refs CHARTS-175.

Charts: Migrate geo-chart container to Stack

e2a8062

Both the loading placeholder and the main chart container are now <Stack> elements with centered alignment. position: relative stays in SCSS because Google Charts' GeoChart needs it for tooltip positioning. Refs CHARTS-175.

Charts: Replace __experimentalHStack with Stack in donut story

0ec48ce

Migrates the CustomPieLegend example off @wordpress/components' experimental HStack onto the stable Stack from @wordpress/ui. Refs CHARTS-175.

Charts: Add changelog for CHARTS-175 Stack migration

66d897c

Refs CHARTS-175.

Charts: Remove unused className from annotation popover header Stack

94fbcb5

The line-chart__annotation-label-popover-header class had no corresponding SCSS rule and was left over from the migration to Stack. Drop it.

adamwoodnz and others added 6 commits April 9, 2026 14:37

Charts: Hoist alignmentToFlex map to module scope in BaseLegend

58e0188

The mapping is static and doesn't depend on props or state. Moving it out of the render body avoids recreating the object on every render.

Charts: Fix regression in alignment of CustomPieLegend Stack

0ed9f37

adamwoodnz force-pushed the charts-175-use-stack-for-layout branch from 4ee6195 to 0ed9f37 Compare April 9, 2026 02:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Charts: Use Stack for layout#47981

Charts: Use Stack for layout#47981
adamwoodnz wants to merge 14 commits intotrunkfrom
charts-175-use-stack-for-layout

adamwoodnz commented Apr 7, 2026 •

edited

Loading

Uh oh!

github-actions bot commented Apr 7, 2026 •

edited

Loading

Uh oh!

github-actions bot commented Apr 7, 2026 •

edited

Loading

Uh oh!

jp-launch-control bot commented Apr 7, 2026 •

edited

Loading

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

adamwoodnz commented Apr 8, 2026

Uh oh!

cursor bot commented Apr 8, 2026 •

edited

Loading

Uh oh!

adamwoodnz commented Apr 8, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

adamwoodnz commented Apr 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Proposed changes

Other information

Related product discussion/links

Does this pull request change what data or activity we track or use?

Testing instructions

Note

Uh oh!

github-actions bot commented Apr 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Apr 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jp-launch-control bot commented Apr 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage Summary

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

adamwoodnz commented Apr 8, 2026

Uh oh!

cursor bot commented Apr 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adamwoodnz commented Apr 8, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

adamwoodnz commented Apr 7, 2026 •

edited

Loading

github-actions bot commented Apr 7, 2026 •

edited

Loading

github-actions bot commented Apr 7, 2026 •

edited

Loading

jp-launch-control bot commented Apr 7, 2026 •

edited

Loading

cursor bot commented Apr 8, 2026 •

edited

Loading