Skip to content

Charts: Replace hardcoded typography with WPDS design tokens#47989

Merged
adamwoodnz merged 20 commits intotrunkfrom
cursor/charts-typography-tokenization-edd2
Apr 9, 2026
Merged

Charts: Replace hardcoded typography with WPDS design tokens#47989
adamwoodnz merged 20 commits intotrunkfrom
cursor/charts-typography-tokenization-edd2

Conversation

@adamwoodnz
Copy link
Copy Markdown
Contributor

@adamwoodnz adamwoodnz commented Apr 8, 2026

Fixes: CHARTS-198

Proposed changes

Replace hardcoded typography values across @automattic/charts with WPDS (WordPress Design System) CSS custom property tokens (--wpds-font-*), so consumers (Jetpack, Woo, Newspack, etc.) can control chart typography centrally through their design system.

This PR also corrects a related architectural issue: chart components should not dictate their own font-family — they should inherit it from the host application's cascade. The original version of this PR added font-family: var(--wpds-font-family-body, sans-serif) to every chart root, which silently overrode host typography whenever the token wasn't defined. That has been reverted; charts now inherit typography the same way any well-behaved UI component does.

Token mapping

Property WPDS Token Fallback
Font family (body) --wpds-font-family-body (inherits from host)
Font size 12px --wpds-font-size-sm 12px
Font size 13px --wpds-font-size-md 13px
Font size 14px --wpds-font-size-md 14px
Font size 16px --wpds-font-size-lg 16px
Font size 18px --wpds-font-size-xl 18px
Font weight regular --wpds-font-weight-regular 400
Font weight medium --wpds-font-weight-medium 499
Line height 16px --wpds-font-line-height-xs 16px
Line height 20px --wpds-font-line-height-sm 20px

On the 499 medium fallback: 499 is the exact value defined by the WPDS token, not a typo. The fallback is intentionally aligned with the design system value rather than the previous 500 hardcode.

On font weights generally: The previous code used arbitrary weights (500, 600, 700) in various places. The WPDS design system only provides regular and medium for body-level typography, so this PR deliberately collapses all of those to medium. This is a small visual change in a few labels/tooltips where text was previously bold; it is intentional alignment with the design system rather than a regression.

Typography scope (chart components do not set font-family)

Chart components intentionally do not declare font-family at their root. Hosts are expected to set typography at a suitable ancestor (typically body), and charts inherit it via normal CSS cascade. This matches how every other well-behaved UI component works and gives the host complete control.

For SVG <text> elements — which inherit font-family from their closest HTML ancestor via standard CSS inheritance — no special handling is required.

For portaled tooltips (which escape the chart's DOM tree via visx's TooltipInPortal), the portal mounts to document.body and inherits from whatever the host sets there. The same rule applies: the host owns typography.

Storybook host simulation

Because charts no longer declare font-family at the root, Storybook needs to act as a WPDS-themed host application so stories render with the design system font. This is done inside the charts package's own StoryChartProvider (in src/stories/chart-decorator.tsx), which wraps stories in a <div style={{ fontFamily: 'var(--wpds-font-family-body, system-ui, sans-serif)' }}>. The shared Jetpack Storybook preview.jsx is untouched.

PieChart label background sizing fix

While reviewing typography, we discovered that PieChart was measuring SVG label widths with only a numeric fontSize — falling back to 12 whenever providerTheme.svgLabelSmall?.fontSize was a string (e.g. '12px') — and never passing fontFamily or fontWeight to getStringWidth. This produced incorrect label background rectangles for themed pie charts.

The numeric fallback logic previously lived inside use-chart-margin as a private helper. It has been extracted into a shared utility (src/utils/resolve-font-size.ts) and is now used by both use-chart-margin and PieChart, which also passes fontFamily and fontWeight through to getStringWidth for accurate measurement.

Breaking changes to be aware of

Two previously-documented custom properties on ConversionFunnelChart have been removed:

  • --funnel-font-family
  • --step-font-family

These are replaced by the standard host-app convention of setting font-family on an ancestor element. Consumers who were setting either variable should migrate to setting font-family (or --wpds-font-family-body) on an ancestor such as body.

Files changed

SCSS modules:

  • conversion-funnel-chart.module.scss — Typography tokenized; removed deprecated --funnel-font-family / --step-font-family; removed font-family from root and child selectors.
  • line-chart.module.scss — Typography tokenized.
  • pie-semi-circle-chart.module.scss — Typography tokenized.
  • base-legend.module.scss — Typography tokenized.
  • base-tooltip.module.scss — Typography tokenized.
  • trend-indicator.module.scss — Typography tokenized.
  • leaderboard-chart.module.scss — Typography tokenized.

TypeScript:

  • pie-chart.tsx — Uses shared resolveFontSize and passes fontFamily / fontWeight to getStringWidth for accurate SVG label measurement.
  • use-chart-margin.tsx — Imports resolveFontSize from the shared utility.
  • utils/resolve-font-size.ts — New shared utility for parsing number | string fontSize values from themes.
  • utils/index.ts — Exports resolveFontSize.
  • stories/chart-decorator.tsx — Wraps chart stories in a host-app <div> with --wpds-font-family-body so Storybook acts as a WPDS-themed host.

Docs / stories:

  • Conversion funnel docs — Removed the CSS custom properties section (deprecated vars removed).
  • Leaderboard docs and story — Removed hardcoded font-family examples.
  • Pie semi-circle docs — Reference WPDS tokens.

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

No.

Testing instructions

  1. pnpm jetpack test js js-packages/charts — 841 tests pass.
  2. pnpm jetpack build js-packages/charts — build succeeds; dist/index.css contains --wpds-font-* tokens.
  3. In Storybook, verify:
    • Charts render using the WPDS font in all stories (the charts package decorator simulates a host app).
    • With no WPDS token and no host typography, charts fall back to inherited browser default.
    • Providing --wpds-font-family-body at body or an ancestor overrides the font as expected.
    • Portaled tooltips (conversion funnel, pie, pie semi-circle) pick up the host's body font correctly.
  4. Verify PieChart label backgrounds size correctly when svgLabelSmall.fontSize is provided as a string like '12px'.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 8, 2026

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 cursor/charts-typography-tokenization-edd2 branch.
  • To test on Simple, run the following command on your sandbox:
bin/jetpack-downloader test jetpack cursor/charts-typography-tokenization-edd2

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
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 8, 2026

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:

  1. Ensure all required checks appearing at the bottom of this PR are passing.
  2. Make sure to test your changes on all platforms that it applies to. You're responsible for the quality of the code you ship.
  3. You can use GitHub's Reviewers functionality to request a review.
  4. 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!

@github-actions 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 8, 2026
@adamwoodnz adamwoodnz removed 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 8, 2026
@adamwoodnz adamwoodnz self-assigned this Apr 8, 2026
@jp-launch-control
Copy link
Copy Markdown

jp-launch-control bot commented Apr 8, 2026

Code Coverage Summary

Coverage changed in 2 files.

File Coverage Δ% Δ Uncovered
projects/js-packages/charts/src/charts/pie-chart/pie-chart.tsx 86/89 (96.63%) 0.04% 0 💚
projects/js-packages/charts/src/hooks/use-chart-margin.tsx 40/40 (100.00%) 4.26% -2 💚

1 file is newly checked for coverage.

File Coverage
projects/js-packages/charts/src/utils/resolve-font-size.ts 10/10 (100.00%) 💚

Full summary · PHP report · JS report

@adamwoodnz adamwoodnz changed the title CHARTS-198: Replace hardcoded typography with WPDS design tokens Charts: Replace hardcoded typography with WPDS design tokens Apr 8, 2026
@adamwoodnz adamwoodnz requested a review from Copilot April 8, 2026 21:29
@adamwoodnz adamwoodnz marked this pull request as ready for review April 8, 2026 21:29
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Replace hardcoded typography across @automattic/charts with WPDS CSS custom property tokens (--wpds-font-*) so consuming products can control typography centrally.

Changes:

  • Update multiple chart/component SCSS modules to use WPDS font family/size/weight/line-height tokens with fallbacks.
  • Adjust PieChart label sizing logic to derive SVG label font size from the theme when estimating label background width.
  • Update Storybook docs/stories to reflect the new WPDS token-based typography approach and add a changelog entry.

Reviewed changes

Copilot reviewed 17 out of 17 changed files in this pull request and generated 13 comments.

Show a summary per file
File Description
projects/js-packages/charts/src/components/trend-indicator/trend-indicator.module.scss Switch trend indicator typography to WPDS token-based font size/weight.
projects/js-packages/charts/src/components/tooltip/base-tooltip.module.scss Switch tooltip font-size to WPDS token with fallback.
projects/js-packages/charts/src/components/legend/private/base-legend.module.scss Switch legend font-size/weight to WPDS tokens with fallbacks.
projects/js-packages/charts/src/charts/sparkline/sparkline.module.scss Add WPDS-based font-family for sparkline container.
projects/js-packages/charts/src/charts/pie-semi-circle-chart/stories/index.docs.mdx Update docs to describe WPDS token-driven label/note typography.
projects/js-packages/charts/src/charts/pie-semi-circle-chart/pie-semi-circle-chart.module.scss Move label/note typography to WPDS tokens with fallbacks.
projects/js-packages/charts/src/charts/pie-chart/pie-chart.tsx Use themed SVG label font size when estimating label background width.
projects/js-packages/charts/src/charts/pie-chart/pie-chart.module.scss Add WPDS-based font-family for pie chart container.
projects/js-packages/charts/src/charts/line-chart/line-chart.module.scss Add WPDS-based font-family and update tooltip/annotation typography tokens.
projects/js-packages/charts/src/charts/leaderboard-chart/stories/index.stories.tsx Remove hardcoded fontFamily from story inline styles.
projects/js-packages/charts/src/charts/leaderboard-chart/stories/index.docs.mdx Remove hardcoded font-family examples from docs.
projects/js-packages/charts/src/charts/leaderboard-chart/leaderboard-chart.module.scss Add WPDS-based font-family and tokenized empty state font-size.
projects/js-packages/charts/src/charts/geo-chart/geo-chart.module.scss Add WPDS-based font-family to geo chart container.
projects/js-packages/charts/src/charts/conversion-funnel-chart/stories/index.docs.mdx Remove docs for old font custom properties and keep theming guidance.
projects/js-packages/charts/src/charts/conversion-funnel-chart/conversion-funnel-chart.module.scss Replace funnel/step font custom properties with WPDS tokens and fallbacks.
projects/js-packages/charts/src/charts/bar-chart/bar-chart.module.scss Add WPDS-based font-family to bar chart container.
projects/js-packages/charts/changelog/charts-typography-tokenization Add changelog entry for typography tokenization.

@adamwoodnz
Copy link
Copy Markdown
Contributor Author

Thanks for the review, Copilot. Consolidating responses to several related comments in this thread:

499 font-weight fallbacks are intentional

The 499 fallback in var(--wpds-font-weight-medium, 499) is not a typo — it matches the exact WPDS design system token value for medium. The design system team defined it that way to preserve sub-pixel rendering parity on certain platforms; we want the fallback to mirror the token rather than diverge from it.

Additionally, the previous hardcoded weights of 500, 600, and 700 in these modules are being deliberately collapsed to the medium token as part of this tokenization pass. Per the design direction, chart typography should only use weights that are actually available in the design system — medium and regular — rather than arbitrary weights. So the apparent "de-emphasis" from 600/700 → medium is the intended outcome, not a regression.

This applies to all the 499 flags across trend-indicator, base-legend, line-chart, pie-semi-circle-chart, conversion-funnel-chart, and the pie-semi-circle docs.

sans-serif fallback on --wpds-font-family-body is intentional

The bar-chart suggestion to use inherit instead of sans-serif was considered but rejected: sans-serif is a deliberately safe default so that charts remain legible in environments where neither WPDS tokens nor the host app's typography cascade reach the chart's root element (e.g. portaled tooltips, SVG sub-trees). This applies to every place --wpds-font-family-body is used.

Conversion funnel: backward compatibility restored

Good catch on the potential breaking change. I've re-added --funnel-font-family and --step-font-family as deprecated fallbacks in the cascade:

font-family: var(--wpds-font-family-body, var(--funnel-font-family, sans-serif));

Consumers already setting either deprecated variable will continue to get their custom font. --wpds-font-family-body takes precedence when set. Both deprecated variables are now documented as such in the conversion funnel's Storybook docs.

pie-chart.tsx label sizing — fixed

Both of your concerns on this one were valid:

  1. providerTheme.svgLabelSmall?.fontSize could be a string like '12px'; the numeric-only check silently fell back to 12. Now using the shared resolveFontSize helper from use-chart-margin, which parses strings.
  2. getStringWidth was only receiving fontSize. It now also receives fontFamily and fontWeight from the theme so the measured background width matches the rendered label.

All 841 tests still pass.

@adamwoodnz adamwoodnz marked this pull request as draft April 8, 2026 23:10
Replace all hardcoded font-family, font-size, font-weight, and line-height
values across chart SCSS modules and components with WPDS (WordPress Design
System) CSS custom property tokens (--wpds-font-*).

SCSS modules updated:
- conversion-funnel-chart: Replace SF Pro font families with --wpds-font-family-body,
  all font sizes with --wpds-font-size-*, weights with --wpds-font-weight-*, and
  line heights with --wpds-font-line-height-*
- line-chart: Tokenize tooltip font-weight and annotation popover font-size
- base-legend: Tokenize legend item font-size and value font-weight
- base-tooltip: Tokenize tooltip font-size
- pie-semi-circle-chart: Tokenize label font-weight/size and note font-size
- trend-indicator: Tokenize font-size and font-weight
- leaderboard-chart: Tokenize empty state font-size

Theme/component updates:
- Default theme svgLabelSmall: Add fontFamily token for SVG text elements
- pie-chart: Wire SVG label fontSize/fontFamily through theme.svgLabelSmall
- Leaderboard story: Remove hardcoded SF Pro Text fontFamily override

Documentation updates:
- conversion-funnel-chart docs: Replace --funnel-font-family/--step-font-family
  with --wpds-font-family-body
- leaderboard-chart docs: Replace font-family examples with WPDS tokens
- pie-semi-circle-chart docs: Reference WPDS tokens for label/note typography

All tokens use current values as fallbacks to preserve visual backward
compatibility when WPDS tokens are not provided by the host environment.
WPDS tokens should be set through ThemeProvider, not raw CSS overrides.
Remove the entire section per review feedback.
Per review feedback, use only sans-serif as the fallback for
--wpds-font-family-body instead of SF Pro or system font stacks.
Also remove WPDS token override from leaderboard CSS docs example.
…on funnel

font-family is an inherited CSS property, so child elements within
.conversion-funnel-chart inherit it from the top-level selector.
Removed the redundant declarations from .main-rate, .change-indicator,
.step-label, .step-rate, .tooltip-title, and .tooltip-content.

Kept font-family on .tooltip-wrapper since tooltips render in a visx
portal outside the .conversion-funnel-chart DOM tree.
WPDS only provides --wpds-font-weight-regular (400) and
--wpds-font-weight-medium (499). There are no tokens for semibold (600)
or bold (700), so these should remain hardcoded rather than being
incorrectly mapped to --wpds-font-weight-medium.
Use only weights available in the design system:
- --wpds-font-weight-medium with fallback 499
- --wpds-font-weight-regular with fallback 400

Previous 500, 600, and 700 values are all mapped to medium (499)
to match the WPDS token definitions.
Apply font-family: var(--wpds-font-family-body, sans-serif) to the
outer selector of all chart SCSS modules so every chart opts in to
the WPDS font-family token. Children inherit automatically.

Charts updated: bar-chart, geo-chart, leaderboard-chart, line-chart,
pie-chart, pie-semi-circle-chart, sparkline.
(conversion-funnel-chart already had it.)
cursoragent and others added 6 commits April 9, 2026 11:41
The fontFamily on svgLabelSmall was redundant: inline SVG <text>
elements inherit font-family from their parent HTML element via CSS
inheritance. Now that every chart root selector sets
font-family: var(--wpds-font-family-body, sans-serif), all SVG text
inside those charts inherits it automatically.

Also removes the explicit fontFamily prop from the pie chart's
slice label <text> element for the same reason.
…aphy

The label background width previously called getStringWidth with only
the numeric fontSize, silently falling back to 12 when svgLabelSmall.fontSize
was a string like '12px', and never passed fontFamily or fontWeight.

Export resolveFontSize from use-chart-margin so it can be reused, then
use it in PieChart and pass fontFamily/fontWeight through to getStringWidth
so the measured background matches the rendered label.

Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
Re-add --funnel-font-family and --step-font-family as fallbacks in the
ConversionFunnelChart cascade so consumers already setting either
variable continue to get their custom font. --wpds-font-family-body
takes precedence when set.

Both variables are now documented as deprecated in the Storybook docs
and will be removed in a future release.

Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
Remove the separate pie-chart and conversion-funnel changelog entries
added alongside the recent fixes. Their contents are covered by the
existing charts-typography-tokenization entry, which is the single
changelog entry for this PR.

Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
…chart roots

Chart components should not dictate their own font-family — that is the
host application's responsibility. Previously each chart module declared
font-family: var(--wpds-font-family-body, sans-serif) at its root, which
silently overrode the host's typography cascade whenever --wpds-font-family-body
was not defined.

Remove the declaration from every chart root (bar, line, pie, pie-semi-circle,
sparkline, geo, leaderboard, conversion-funnel) and from the conversion-funnel
tooltip-wrapper. Charts now inherit the font-family set by the host page or
component, which is the standard UI component convention.

Also drop the short-lived backward-compatibility layer for the deprecated
--funnel-font-family and --step-font-family custom properties and its docs
section, since with no font-family at the chart root there is no longer a
natural home for the fallback chain.

Consumers relying on the implicit sans-serif fallback will now inherit from
their host page. Consumers relying on the deprecated funnel custom properties
should set font-family on an ancestor element (typically body) via their own
theme layer.

Storybook now sets body font-family to --wpds-font-family-body, acting as a
WPDS-themed host so stories render with the design system font without the
chart components themselves declaring it.

Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
The previous commit set body font-family in the shared Storybook SCSS,
which would have affected every package's stories. Move the rule to a
.charts-storybook-host wrapper applied by a decorator that only fires
for stories titled under "JS Packages/Charts Library", so charts still
render with the design system font without leaking typography changes
into other packages' stories.

Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
@adamwoodnz adamwoodnz force-pushed the cursor/charts-typography-tokenization-edd2 branch from 507a361 to 393ceb5 Compare April 8, 2026 23:45
…rator

The previous commit applied the WPDS body font via a decorator and CSS
class in the shared Storybook preview. That worked but put charts-specific
concerns into code shared with every other package's stories.

Move the host-app font-family wrapper into the charts package's own
shared decorator (StoryChartProvider, which backs both chartDecorator
and simpleChartDecorator). Charts stories now render inside a div with
font-family: var(--wpds-font-family-body, system-ui, sans-serif) without
touching the shared storybook preview.jsx or style.scss.

Revert the shared Storybook changes from the previous commit so the
host-app simulation lives entirely in the charts package.

Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
@adamwoodnz adamwoodnz force-pushed the cursor/charts-typography-tokenization-edd2 branch from 393ceb5 to f8be364 Compare April 8, 2026 23:48
resolveFontSize was defined locally inside use-chart-margin and then
re-exported through the hooks barrel so pie-chart could reuse it for
label sizing. Now that there are multiple call sites, move it into its
own file under src/utils alongside the other chart helpers and export
it from the utils barrel.

Drop the re-export from the hooks barrel and update pie-chart to import
directly from utils.

Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
@adamwoodnz
Copy link
Copy Markdown
Contributor Author

@claude review please

@claude
Copy link
Copy Markdown
Contributor

claude bot commented Apr 9, 2026

Claude finished @adamwoodnz's task in 4m 25s —— View job


Code Review

  • Read changed files
  • Review SCSS typography tokenization
  • Review resolve-font-size.ts utility
  • Review pie-chart.tsx label sizing
  • Review use-chart-margin.tsx changes
  • Review Storybook decorator changes
  • Post final review

Overall this is well-executed. The tokenization is thorough, the architecture decision to inherit font-family from the host is sound, and the resolveFontSize extraction is a clean improvement. A few things worth addressing before merge:

Medium: --wpds-font-size-md has two different fallback values

The same token is used with both 13px and 14px (and 0.875rem) as fallbacks across different files:

  • conversion-funnel-chart.module.scssvar(--wpds-font-size-md, 13px) (in .change-indicator, .step-rate, .tooltip-content)
  • base-tooltip.module.scss, leaderboard-chart.module.scss, line-chart.module.scss, pie-semi-circle-chart.module.scssvar(--wpds-font-size-md, 14px)
  • trend-indicator.module.scss, base-legend.module.scssvar(--wpds-font-size-md, 0.875rem)

When WPDS tokens are present, everything resolves to one canonical value — that's fine. But when tokens are absent, .change-indicator renders at 13px while .tooltip-wrapper .tooltip-content (in the same chart) renders at 13px while a nearby <BaseTooltip> renders at 14px. The fallback inconsistency will only matter in no-WPDS environments, but it should be a conscious choice. If 13px was the intentional original value for the funnel (it was), a comment saying so would make this easier to follow: Fix this →

Medium: svgLabelSmall.fontFamily / fontWeight are always undefined in the default theme

pie-chart.tsx:444-449 now passes fontFamily and fontWeight from svgLabelSmall to getStringWidth:

const svgLabelSmall = providerTheme.svgLabelSmall;
const fontSize = resolveFontSize( svgLabelSmall?.fontSize ) ?? 12;
const estimatedTextWidth = getStringWidth( arc.data.label, {
    fontSize,
    fontFamily: svgLabelSmall?.fontFamily,
    fontWeight: svgLabelSmall?.fontWeight,
} );

But themes.ts:29 only sets fill on svgLabelSmall:

svgLabelSmall: { fill: 'var(--jp-gray-80, #2c3338)' },

So fontFamily and fontWeight are always undefined for the default theme. The measurement improvement only benefits consumers who supply a custom svgLabelSmall with those properties. The commit message says "passes fontFamily and fontWeight... for accurate measurement" but in the default case, accuracy is unchanged from before (modulo the resolveFontSize improvement, which is the real fix here). This isn't a regression — just worth noting that the stated benefit is currently theoretical.

Low: resolveFontSize silently mishandles rem values

parseFloat("0.875rem") returns 0.875, not 14. If theme font sizes are ever expressed in rem (e.g., svgLabelSmall: { fontSize: '0.875rem' }), this would produce a nearly-zero margin in use-chart-margin (0.875 + 8 = ~9px instead of ~22px) and a near-invisible label background in pie-chart.tsx.

Currently safe because themes.ts only sets fill on svgLabelSmall, so the fallback to DEFAULT_FONT_SIZE = 12 applies. But it's a latent trap. The JSDoc says "Returns undefined when the value is missing or cannot be parsed" — a rem value can be parsed (returns 0.875) so the caller doesn't know it's wrong. At minimum, a JSDoc note that only px values are supported, or a unit check that rejects non-px strings: Fix this →

Low: id="app" in StoryChartProvider can cause duplicate IDs

chart-decorator.tsx:166 wraps the story in <div id="app">. On a Storybook docs page that renders multiple stories side-by-side, this produces duplicate id attributes — invalid HTML and a potential selector bug. Changing to className="app" or removing the attribute entirely would be safer.

Low: No unit tests for resolveFontSize

Code coverage shows 71.43% (5/7 lines). The untested branches are likely the isNaN(parsed) path and the final return undefined. Given this utility is now used in two different measurement paths, a small dedicated test file would add confidence. Fix this →


Minor observations (no action needed)

  • --funnel-font-family / --step-font-family removal: The PR description correctly documents this as a breaking change. The in-thread comment about "backward compat restored" refers to an earlier iteration that was subsequently reverted — the description's breaking change section is the authoritative state.
  • 499 fallback: Correctly intentional per the design system definition; the PR description explains it well.
  • Portaled tooltip font inheritance: Relying on body for font inheritance is architecturally correct; the PR description is clear about the host contract.
  • Storybook system-ui fallback: var(--wpds-font-family-body, system-ui, sans-serif) in the decorator is fine — different from the sans-serif used in SCSS, but the decorator is a Storybook-specific host simulation, not production CSS.

@adamwoodnz adamwoodnz marked this pull request as ready for review April 9, 2026 00:21
@adamwoodnz adamwoodnz requested a review from a team April 9, 2026 00:22
kangzj
kangzj previously approved these changes Apr 9, 2026
Copy link
Copy Markdown
Contributor

@kangzj kangzj left a comment

Choose a reason for hiding this comment

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

Looks good and tests well. A big step forward 👍

visx's buildChartTheme injects a hardcoded font stack
(-apple-system,BlinkMacSystemFont,Roboto,Helvetica Neue,sans-serif)
into every axis tickLabel and axisLabel style, which visx's Axis
components then apply as an inline style on SVG <text> elements.
Inline styles beat inherited CSS, so axis text was silently ignoring
the host application's font-family cascade regardless of what the
chart roots did.

Set fontFamily: 'inherit' on svgLabelSmall and svgLabelBig in the
default theme so the inline value emitted by visx is 'inherit',
letting SVG text actually pick up the host's font-family via normal
CSS inheritance — consistent with the rest of this PR's decision
that chart components should not dictate typography.

Expose svgLabelBig on ChartTheme so it can be overridden alongside
svgLabelSmall.

Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
adamwoodnz and others added 4 commits April 9, 2026 13:38
Earlier in this PR we standardized on a single sans-serif fallback for
--wpds-font-family-body across every chart SCSS module. The Storybook
decorator drifted from that policy by including system-ui in the middle
of the chain. Align it with the rest of the PR so the fallback policy
is consistent everywhere the variable is used.

The fallback is essentially defensive — design-tokens.css is imported
unconditionally by Storybook's preview, so the var should always
resolve in practice.

Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
The same WPDS token was being used with three different fallback
values across the package: 13px (conversion-funnel, svg-empty-state),
14px (line-chart, leaderboard, base-tooltip, pie-semi-circle), and
0.875rem (trend-indicator, base-legend). When WPDS tokens are present
they all resolve to the same canonical value, but in no-WPDS
environments the inconsistency produced visibly different sizes
between adjacent components in the same chart.

Standardize on 13px — the actual WPDS design system value for
--wpds-font-size-md — so the fallback matches the token everywhere
and matches the policy already used by the conversion funnel and
empty state. Update the pie-semi-circle docs to reflect the new
default.

Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
The previous implementation used parseFloat() on any string input,
which silently accepted relative units like 0.875rem and returned
the unitless prefix (0.875). Callers in use-chart-margin and
pie-chart would then treat that as a pixel value, producing
nearly-zero margins or invisible label backgrounds.

Tighten the parser to accept only plain numeric strings and pixel
values, rejecting rem, em, %, vh, vw, and CSS keywords like inherit
or medium. The function now matches its JSDoc contract: returns
undefined for any value it cannot reliably resolve to absolute
pixels, so callers fall back to their own defaults.

Add a dedicated unit test file covering numeric input, pixel
strings, whitespace trimming, NaN, undefined, empty strings,
unparseable input, and the previously-accepted relative units that
should now be rejected.

Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
The id was added as a label of intent for the host-app font wrapper
div, but Storybook docs pages render multiple stories side-by-side
in the same DOM, which would produce duplicate id attributes —
invalid HTML and a potential selector collision.

The wrapper's purpose is documented by the adjacent comment, so the
id is unnecessary. Remove it.

Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
@adamwoodnz
Copy link
Copy Markdown
Contributor Author

adamwoodnz commented Apr 9, 2026

Thanks for the review @claude! Addressed in three follow-up commits:

  • Medium #1 (font-size-md fallback inconsistency) — Fixed in eaf6d2ad68. Standardized all --wpds-font-size-md fallbacks to 13px (the actual WPDS design system value), removing the 14px and 0.875rem variants. Now consistent across base-tooltip, base-legend, line-chart, leaderboard, pie-semi-circle, trend-indicator, and the conversion-funnel chart.
  • Low #1 (resolveFontSize silently mishandles rem) — Fixed in 046738faf0. Tightened the parser to accept only plain numeric strings and px values; rejects rem, em, %, vh, vw, and CSS keywords. Added a dedicated unit test file with 12 cases covering numeric input, pixel strings, whitespace, NaN, undefined, empty strings, unparseable input, and the previously-accepted relative units that should now be rejected. Also covers Low Create a readme.md for the Jetpack GitHub Repository. #5 (no unit tests).
  • Low #2 (id="app" duplicate IDs) — Fixed in 8f9618ce68. The id was added as a label of intent but Storybook docs pages render multiple stories side-by-side, so it would produce invalid duplicate ids. Removed it; the wrapper's purpose is documented by the adjacent comment.

Medium #2 (svgLabelSmall.fontFamily/fontWeight pass-through is theoretical) — Skipping. You're right that for the default theme this is currently a no-op, and 'inherit' (now set on svgLabelSmall.fontFamily in 079ced1162) is meaningless to canvas-based getStringWidth. But the pass-through is correct for any consumer who supplies a custom theme with concrete fontFamily/fontWeight strings — it's a guardrail for that case, not dead code. Removing it would silently regress measurement accuracy for those consumers.

Minor observation about system-ui in the decorator — That had already been removed in cc4f37dabf (your review was running before that landed). Decorator now uses var(--wpds-font-family-body, sans-serif), consistent with the SCSS fallback policy.

All 853 tests pass.

@claude
Copy link
Copy Markdown
Contributor

claude bot commented Apr 9, 2026

Claude finished @adamwoodnz's task in 1m 5s —— View job


Follow-up Review

  • Read trigger comment
  • Verify Medium #1: --wpds-font-size-md fallback standardized to 13px
  • Verify Low #1: resolveFontSize rejects rem/em, unit tests added
  • Verify Low #2: id="app" removed from chart decorator
  • Verify Medium #2 explanation (skip)

All three fixes confirmed correct. Here's what I checked:

Medium #1 — Every --wpds-font-size-md usage in SCSS is now 13px across all 10 locations (base-tooltip, base-legend, line-chart, leaderboard, pie-semi-circle, trend-indicator, svg-empty-state, and all three conversion-funnel selectors). Consistent.

Low #1resolveFontSize.ts uses ^(-?\d+\.?\d*|-?\.\d+)(px)?$ which correctly accepts bare integers, decimals, and px suffixes, and rejects rem, em, %, vh, vw by requiring the string to end immediately after the optional px. The test file covers all the cases: numbers (including NaN), px strings, whitespace trimming, relative unit rejection, CSS keywords, empty strings, and undefined. Solid.

Low #2chart-decorator.tsx:169 wraps in <div style={{ fontFamily: '...' }}> with no id attribute. Fixed.

Medium #2 skip — Agreed. fontFamily: 'inherit' is now set on svgLabelSmall in themes.ts with a clear comment explaining it overrides visx's hardcoded font stack on SVG <text> elements. Passing it to getStringWidth is a no-op for the default theme (canvas doesn't resolve inherit), but the pass-through is the right guardrail for consumers with concrete font names in custom themes. No regression.

This is ready to merge.

@adamwoodnz adamwoodnz merged commit 0dc3e76 into trunk Apr 9, 2026
88 checks passed
@adamwoodnz adamwoodnz deleted the cursor/charts-typography-tokenization-edd2 branch April 9, 2026 02:25
@github-actions github-actions bot removed the [Status] Needs Review This PR is ready for review. label Apr 9, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants