Charts: Tokenize spacing and border values with WPDS dimension/border tokens#48019
Charts: Tokenize spacing and border values with WPDS dimension/border tokens#48019adamwoodnz merged 9 commits intotrunkfrom
Conversation
|
Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.
Interested in more tips and information?
|
|
Thank you for your PR! When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:
This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖 Follow this PR Review Process:
If you have questions about anything, reach out in #jetpack-developers for guidance! |
adamwoodnz
changed the base branch from
trunk
to
charts-175-use-stack-for-layout
Code Coverage SummaryThis PR did not change code coverage! That could be good or bad, depending on the situation. Everything covered before, and still is? Great! Nothing was covered before? Not so great. 🤷 |
There was a problem hiding this comment.
Pull request overview
This PR updates the Charts package styling to rely on WPDS dimension/border tokens instead of hardcoded spacing/border values, keeping rendered output stable via pixel fallbacks and making a couple of small layout adjustments in the conversion funnel chart.
Changes:
- Tokenize padding/margin/gap and border-radius/border-width values across tooltip, legend, line chart, leaderboard chart, and conversion funnel chart styles.
- Adjust conversion funnel empty-state layout to center content via
Stackalignment and refine step label/rate layout via aStack+ typography tweaks. - Add a changelog fragment documenting the styling/tokenization update.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| projects/js-packages/charts/src/components/tooltip/base-tooltip.module.scss | Tokenizes tooltip padding and border radius with WPDS tokens and pixel fallbacks. |
| projects/js-packages/charts/src/components/legend/private/base-legend.module.scss | Tokenizes focus-ring border radius with WPDS token + fallback. |
| projects/js-packages/charts/src/charts/line-chart/line-chart.module.scss | Tokenizes tooltip/popover spacing and border radius; aligns spacing fallbacks to pixel values. |
| projects/js-packages/charts/src/charts/leaderboard-chart/leaderboard-chart.module.scss | Tokenizes label/empty-state padding; documents intentional non-tokenized 6px row-gap. |
| projects/js-packages/charts/src/charts/conversion-funnel-chart/conversion-funnel-chart.tsx | Centers empty-state via Stack alignment; wraps step label/rate in a Stack with tokenized gap. |
| projects/js-packages/charts/src/charts/conversion-funnel-chart/conversion-funnel-chart.module.scss | Tokenizes border radius/border width/padding; updates label/rate typography and truncation behavior; removes empty-state padding. |
| projects/js-packages/charts/changelog/charts-199-spacing-border-tokenization | Adds changelog entry for WPDS tokenization work. |
adamwoodnz
force-pushed
the
charts-175-use-stack-for-layout
branch
from
0ed9f37 to
58b79c3
Compare
… tokens Replace hardcoded padding, margin, border-radius, and border-width values in module SCSS with --wpds-dimension-* and --wpds-border-* tokens (fallbacks preserved). Scope is the package's component SCSS — story and test scaffolding is unchanged. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The previous 10px value sat between two WPDS padding tokens (sm = 8px, md = 12px) and was left hardcoded in the initial tokenization pass. Round up to padding-md so the separator below the date keeps a touch more breathing room when no WPDS provider is active. Visual delta is 2px in the fallback path. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Replace rem-based fallbacks (0.5rem, 1rem) with the literal pixel values defined by the WPDS dimension tokens (8px, 16px). The previous fallbacks would only resolve to the spec value when the document root font-size is the default 16px; pinning the fallback removes that dependency and matches what the WPDS provider actually emits. No visual change for browsers using the default root font-size.
The 2px margin between the funnel step label and rate had no matching WPDS dimension token, so the initial tokenization pass left it hardcoded. Bumping the label's line-height from line-height-xs (16px) to line-height-sm (20px) absorbs the spacing into the line box: the label gains 2px below its text glyphs, replacing the previous margin, and inter-text gap between label and rate is preserved at ~7.5px. Side effect: the label/rate group is 2px taller overall, shifting the funnel bar down by 2px. Verified in Storybook against the default funnel story.
Replace the inflated label line-height from the previous commit with a cleaner approach: wrap the funnel step label and rate in a Stack with gap-xs (4px), and use line-height-xs (16px) on both. Inter-text gap between label and rate is preserved at ~7.5px; the label/rate group is 2px shorter than the original layout, so the funnel bar shifts up 2px. With the Stack handling vertical layout, the label and rate spans no longer need explicit `display: block` (flex items are blockified by the flexbox spec), `margin: 0` (spans have no default margin), or `font-style: normal` (default). Drop those redundant rules. Also add `white-space: nowrap` to step-label so its existing `overflow: hidden; text-overflow: ellipsis` actually produces an ellipsis on overflow. Without nowrap, the ellipsis declaration is silently a no-op — the text wraps to multiple lines and gets clipped instead of truncated.
The funnel empty state previously used `padding: 48px 24px` to push its text away from the container edges, but it sat at the top of the chart's fixed-height column rather than being vertically centered. The 48px vertical padding was also the only spacing literal in the package with no matching WPDS dimension token. Add `align="center" justify="center"` to the parent Stack in the empty-state branch so the message is centered both axes within the chart's resolved height, matching the centering pattern used by SvgEmptyState. Drop the now-unused padding from .empty-state. Verified centering against the EmptyData story: chart bounds (999.5, 492.75) and empty-state bounds match exactly on both axes.
The leaderboard's bar rows use a compact bar + gap + bar rhythm (6px bar, 6px row-gap, 6px bar) that does not align to the WPDS dimension scale — the closest token (gap-sm = 8px) visibly breaks the balance between row spacing and bar thickness. Add a comment so the next person to look at this doesn't try to tokenize the value. Surfaced during the spacing/border tokenization pass for CHARTS-199.
adamwoodnz
force-pushed
the
charts-199-spacingborder-tokenization
branch
from
b7e00ef to
c3d2512
Compare
|
@claude review please |
|
Claude finished @adamwoodnz's task in 2m 25s —— View job Code Review
This is a clean, well-scoped tokenization PR. The design decisions are clearly documented, fallbacks are correctly anchored to pixel literals, and the intentional outliers (leaderboard Minor: Changelog entry missing
|
| line-height: var(--wpds-font-line-height-xs, 16px); | ||
| margin: 0 0 2px 0; | ||
| display: block; | ||
| white-space: nowrap; |
There was a problem hiding this comment.
Completes the overflow handling styles. Without this text will wrap and not get ellipsis.
There was a problem hiding this comment.
Fixes for text with spaces.
| Before | After |
|---|---|
 |
 |
|
|
||
| .empty-state { | ||
| text-align: center; | ||
| padding: 48px 24px; |
There was a problem hiding this comment.
Replaced with a centered layout, matching other charts, eg. GeoChart
Without an explicit margin, the fixed-position tooltip popover inherits margin: auto, which offsets it from the trigger button. The margin also creates the intended gap between the trigger and the popover, making --wpds-dimension-gap-sm the right token. Document this inline so the declaration isn't mistaken for a hardcoded spacing workaround in future tokenization passes.
|
@claude again |
|
Claude finished @adamwoodnz's task in 1m 57s —— View job Code Review
Both concerns raised in the previous review have been addressed:
A few fresh observations below.
|
adamwoodnz
added
[Status] Needs Review
| // Outlier: not tokenized. The leaderboard's compact bar + gap + bar | ||
| // rhythm (6px bar, 6px row-gap, 6px bar) is intentional and does not | ||
| // map to any WPDS dimension token — gap-sm (8px) breaks the visual | ||
| // balance between the rows and the bars. |
kangzj
left a comment
kangzj
left a comment
There was a problem hiding this comment.
Works as described and changes look good 👍
One issue that's not related to the PR:
- The glyphs are off the lines
- The glyphs render on top of the marker content, which feels odd. I'm not sure what should be the expected behaviour though.
Screen.Recording.2026-04-16.at.5.07.38.PM.mov
dognose24
left a comment
dognose24
left a comment
There was a problem hiding this comment.
LGTM and works as expected from my testing! 👍🏼
| line-height: var(--wpds-font-line-height-sm, 20px); | ||
| margin: 0; | ||
| display: block; | ||
| line-height: var(--wpds-font-line-height-xs, 16px); |
| line-height: var(--wpds-font-line-height-xs, 16px); | ||
| margin: 0 0 2px 0; | ||
| display: block; | ||
| white-space: nowrap; |
There was a problem hiding this comment.
Fixes for text with spaces.
| Before | After |
|---|---|
|
|
|
|
||
| .empty-state { | ||
| text-align: center; | ||
| padding: 48px 24px; |
| return ( | ||
| <Stack | ||
| direction="column" | ||
| align="center" |
| > | ||
| { /* Step Label and Rate */ } | ||
| <div> | ||
| <Stack direction="column" gap="xs"> |
adamwoodnz
added
[Status] Ready to Merge
github-actions
Bot
removed
the
[Status] Ready to Merge
Fixes CHARTS-199
Proposed changes
padding,margin,gap) in chart module SCSS with--wpds-dimension-padding-*and--wpds-dimension-gap-*design tokens.border-radius: 4pxvalues with--wpds-border-radius-md(note: WPDSborder-radius-md= 4px; the suggestedborder-radius-smin the Linear ticket is actually 2px).1pxborder width on the funnel tooltip wrapper with--wpds-border-width-xs.0.5rem→8px,1rem→16px) so the rendered value doesn't depend on the document root font-size.step-label2px margin → wrap label and rate in aStackwithgap-xs(4px) and useline-height-xson both. Bonus: drop redundantdisplay: block/margin: 0/font-style: normalrules (flex items are auto-blockified) and add the missingwhite-space: nowrapso the label's existingtext-overflow: ellipsisactually truncates.48px 24pxpadding → addalign="center" justify="center"to the parentStackso the message is centered both axes within the chart's resolved height (matching theSvgEmptyStatepattern). The 48px literal (which had no token) goes away entirely.padding-bottom: 10px→ tokenized topadding-md(12px), the closer of the two adjacent tokens (sm = 8px, md = 12px).node_modules/@wordpress/theme/src/prebuilt/css/design-tokens.css.Intentionally untouched
row-gap: 6px— the leaderboard's bar rows use a compactbar + gap + barrhythm (6px bar, 6px row-gap, 6px bar) that doesn't align to the WPDS dimension scale; the closest token (gap-sm= 8px) visibly breaks the balance. Documented inline as an outlier.--a8c--charts--leaderboard--bar--border-radius— the legacy CSS variable is left for CHARTS-201.stories//test/files is demo-only and out of scope for the consumer-facing API.Related product discussion/links
Does this pull request change what data or activity we track or use?
No.
Testing instructions
projects/js-packages/charts:pnpm test— all 853 tests should pass.Charts → Tooltip → Default— base tooltip padding/border-radius unchanged.Charts → Line Chart → Annotations— open an annotation popover; padding, margin, and border-radius unchanged.Charts → Conversion Funnel Chart → Default— bar/funnel-bar border-radius unchanged. Hover a bar to confirm tooltip-wrapper padding/border-radius/border-bottom unchanged. The funnel bar position shifts up ~2px relative to trunk because the label/rate group is now slightly more compact (line-height-xs on both, gap-xs Stack between them).Charts → Conversion Funnel Chart → EmptyData— the "No data available" message is now centered both axes within the chart's height (previously sat near the top with 48px/24px padding).Charts → Leaderboard Chart → Empty Data— empty state padding (32px 16px) unchanged.Charts → Legend → *— focus a legend item with the keyboard; the focus-ring border-radius is unchanged..tooltippadding) and confirm the resolved value matches the previous literal. The WPDS provider is not active in default Storybook, so the CSS fallback should drive the rendered value.