Charts: Minimal dynamic X-axis margin in LineChart to fit tick labels #45815

chriskmnds · 2025-11-07T14:29:45Z

Fixes https://linear.app/a8c/issue/M4R73CH-1201/v2-hd-charts-on-monitoring-page-touch-the-card-border.

Proposed changes:

Make the X-axis margin minimal and dynamic, computing it as fontSize + tickLength based on the theme's axisStyles.x.* / svgLabelSmall, with documented defaults when not present.
For a bottom X-axis, use bottom = max(DEFAULT_MARGIN_BOTTOM, fontSize + tickLength); for a top X-axis, use top = max(DEFAULT_MARGIN_TOP, fontSize + tickLength) and a small fixed bottom = DEFAULT_BOTTOM_FOR_TOP_AXIS (10px).
Keep the existing Y-axis behavior (using getLongestTickWidth) but introduce documented constants (DEFAULT_Y_TICK_WIDTH, DEFAULT_MARGIN_*) instead of magic numbers.
Extract X-axis font/tick logic into helpers (resolveFontSize, getXAxisLabelMetrics) to simplify useChartMargin and align the structure with the Y-axis logic.

Other information:

Have you written new tests for your changes, if applicable? (Updated use-chart-margin tests to cover bottom/top X-axis cases.)
Have you checked the E2E test CI results, and verified that your changes do not break them? (Charts package tests pass; no E2E impact expected.)
Have you tested your changes on WordPress.com, if applicable (if so, you'll see a generated comment below with a script to run)?

Jetpack product discussion

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

No.

Testing instructions:

In the charts package:
- Run pnpm -C projects/js-packages/charts test and verify tests pass.
- Run pnpm -C projects/js-packages/charts storybook and open Storybook.
  - Inspect LineChart stories with the default theme and confirm X-axis tick labels do not touch the container edge.
  - Increase the axis label font size (or use a theme with larger tick labels) and confirm the bottom/top margin grows just enough to fit labels without adding excessive padding.
  - Set axis.x.orientation = 'top' in a LineChart story and verify the top margin increases appropriately while the bottom margin is a small 10px buffer.

github-actions · 2025-11-07T14:30:38Z

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 update/fix-chart-x-axis-margins branch.
To test on Simple, run the following command on your sandbox:

bin/jetpack-downloader test jetpack update/fix-chart-x-axis-margins

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 · 2025-11-07T14:30:56Z

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 a "[Type]" label (Bug, Enhancement, Janitorial, Task).
✅ 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 · 2025-11-07T14:43:42Z

Code Coverage Summary

Coverage changed in 1 file.

File	Coverage	Δ%	Δ Uncovered
projects/js-packages/charts/src/hooks/use-chart-margin.tsx	45/47 (95.74%)	-4.26%	2 ❤️‍🩹

Full summary · PHP report · JS report

kangzj · 2025-11-10T03:57:18Z

projects/js-packages/charts/src/hooks/test/use-chart-margin.test.tsx

-		expect( result.current.top ).toBe( 20 );
+		// 12px font → ~15px line-height + 8 tick length + 8 padding = 31
+		expect( result.current.top ).toBe( 31 );
 		expect( result.current.bottom ).toBe( 10 );


FAIL src/hooks/test/use-chart-margin.test.tsx
● useChartMargin › sets top and bottom margin for top x axis

expect(received).toBe(expected) // Object.is equality Expected: 10 Received: 20 105 | // 12px font → ~15px line-height + 8 tick length + 8 padding = 31 106 | expect( result.current.top ).toBe( 31 ); > 107 | expect( result.current.bottom ).toBe( 10 ); | ^ 108 | } ); 109 | 110 | it( 'returns default margin if no axis options', () => { at Object.toBe (src/hooks/test/use-chart-margin.test.tsx:107:35)

Feel like bottom should be 20 here, but I'll leave it to @chihsuan for a second opinion.

I think we should set it to 10, since having the orientation as top eliminates the need for a large bottom margin.

IMO, this line is unnecessary.

Thanks for flagging this. The X‑axis margin logic has been updated so that for a top x‑axis the bottom margin is always DEFAULT_BOTTOM_FOR_TOP_AXIS = 10, independent of the dynamic computation; the test now asserts top === 20 (12px font + 8px tick) and bottom === 10, and it passes with the new implementation.

chihsuan

Thanks for the PR! @chriskmnds I like the idea of respecting theming and different font sizes. 🙂

However, I think we should think more carefully about whether we want to add extra padding.

Before this PR, what we had was:

Now we have:

It does make sense to add padding between each chart and its container (card). However, IMO, it's better to let consumers decide how much padding they want between the chart and its container.

Additionally, this change might cause issues with Woo Analytics widgets, since we don't currently define explicit margins for the charts.

In other words, I think we should adjust the x-axis margin dynamically, but keep it just sufficient to prevent the chart from overlapping its container. If additional space is needed, consumers can add padding to the container as desired. Does that make sense?

chihsuan · 2025-11-10T08:23:06Z

projects/js-packages/charts/src/hooks/use-chart-margin.tsx

+		const xTickLength =
+			// Prefer axis-specific tick length when present, else theme fallback
+			xAxisStyles?.tickLength ?? themeWithX.tickLength ?? 8;
+		const xPadding = 8;


Would it be possible to move the hardcoded values (such as padding, line height, and default settings) into named constants, and add comments that clarify their purposes? This would improve readability and maintainability.

Done. All previously hardcoded values have been moved into documented constants at the top, with brief comments describing what each controls. The hook body now refers to these constants rather than inline numbers.

chihsuan · 2025-11-10T08:24:12Z

projects/js-packages/charts/src/hooks/use-chart-margin.tsx

+				return isNaN( parsed ) ? undefined : parsed;
+			}
+			return undefined;
+		};


Can we move this function out of useChartMargin? The hook is getting a bit too complex. It would be nice to split it up into smaller functions. An idea would be to extract all font size calculation logic into a separate function, similar to how we handle the Y axis.

Agreed, that block was getting heavy. I’ve extracted the X‑axis font/tick logic into resolveFontSize( ... ) and getXAxisLabelMetrics( theme, orientation ), so useChartMargin now just calls the helper and then combines the metrics into margins. This mirrors the way Y‑axis width is delegated to getLongestTickWidth and keeps the hook itself much simpler.

chihsuan · 2025-11-10T08:27:07Z

projects/js-packages/charts/src/hooks/use-chart-margin.tsx

+		if ( xOrientation === 'top' ) {
+			// Preserve a small bottom margin for layout breathing room
+			defaultMargin.top = Math.max( defaultMargin.top, computedXMargin );
+			defaultMargin.bottom = Math.max( defaultMargin.bottom, 10 );


This is consistently assigned the value of defaultMargin.bottom because defaultMargin.bottom is 20. I think we should set the defaultMargin.bottom to 10.

You’re right. Math.max( defaultMargin.bottom, 10 ) was effectively always 20. I’ve introduced DEFAULT_BOTTOM_FOR_TOP_AXIS = 10 and, in the top‑axis case, we now assign defaultMargin.bottom = DEFAULT_BOTTOM_FOR_TOP_AXIS directly. The bottom margin for a top X‑axis is now 10px instead of 20px.

chriskmnds · 2025-11-10T09:45:06Z

Thanks for reviewing! @kangzj @chihsuan

It does make sense to add padding between each chart and its container (card). However, IMO, it's better to let consumers decide how much padding they want between the chart and its container.

There is a point to that, how a "raw" chart component would have zero paddings/margins. I am just proposing this as something done similarly for y-axis. Happy to abandon direction - in fact, I'd love it if one of you would take over if you'd like to entertain this further (being as it is that a shole team is in charge of the charts). I created a PR as I feared just an issue might sit longer in backlog.

Let me know what your final take is - push forward, abandon, or leave it to your hands? :-)

kangzj · 2025-11-17T02:23:15Z

I tracked it with CHARTS-134: Optimize bottom/top margin to fit tick labels. I'll take a look this week. Thanks!

chriskmnds · 2025-11-17T14:13:59Z

Thanks again for the review and for raising the concerns about extra padding and theme consistency. The useChartMargin is updated so the X‑axis margin is now the minimum needed to fit tick labels (fontSize + tickLength), using the existing visx axisStyles.x / svgLabelSmall information, and documented defaults added to the top. The Y‑axis defaults are aligned to use the same pattern. This keeps default layouts effectively unchanged, avoids adding new visual padding at the library level, and still scales correctly for different themes and font sizes. Any additional spacing around the chart is left to consumers via their own container padding/margins.

Please review. Next, will run some tests and update the test cases to cover.

kangzj

The PR look good and tests as described 👍

I don't see we allow axisStyles in our theme - the option is available for XYChartTheme only the moment, which means customized fontSize is not yet supported with our exposed API. However, if supported, we will need this.

I think we can merge the PR and then we could consider support for the ability to customize styles for axis in some ways(might not be necessary though. not 100% sure). It seems we will need our version of buildChartTheme. We should probably get rid of XYChart in the long run because we don't have full control over some functionalities otherwise.

Not sure what you think @adamwoodnz.

… tick labels

… defaults

adamwoodnz · 2025-11-20T03:46:55Z

Thanks again for working on this.

Increase the axis label font size (or use a theme with larger tick labels) and confirm the bottom/top margin grows just enough to fit labels without adding excessive padding.

I increased the axis label font size using svgLabelSmall: { fontSize: '20px' }, in the default theme and the labels have blown out so I'm not sure this is working as intended:

Axis labels aren't something we've spent much time on yet in terms of theming, so that's the only lever we have at present. As @kangzj mentioned we don't support theme axisStyles yet so maybe that needs to be added as a more robust way of controlling the label font size.

chriskmnds · 2025-11-20T05:49:35Z

Thanks @kangzj @adamwoodnz !

I did notice something odd about axisStyles, but this is something already utilised in the code for the y-axis.

It sounds like I may need to give better focus here. Most definitely didn't test much. Thanks for testing!

chriskmnds mentioned this pull request Nov 7, 2025

v2 HD: Fix chart margins on monitoring page Automattic/wp-calypso#106991

Merged

9 tasks

chriskmnds requested a review from a team November 7, 2025 14:30

chriskmnds requested a review from chihsuan November 7, 2025 14:30

github-actions bot added [JS Package] Charts [Tests] Includes Tests RNA labels Nov 7, 2025

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 Nov 7, 2025

taipeicoder requested a review from adamwoodnz November 7, 2025 14:31

kangzj reviewed Nov 10, 2025

View reviewed changes

chihsuan reviewed Nov 10, 2025

View reviewed changes

kangzj self-assigned this Nov 17, 2025

kangzj mentioned this pull request Nov 17, 2025

Optimize bottom/top margin to fit tick labels woocommerce/woocommerce#61980

Closed

kangzj removed their assignment Nov 17, 2025

chriskmnds force-pushed the update/fix-chart-x-axis-margins branch from 9432b9d to bc041d8 Compare November 17, 2025 12:41

chriskmnds changed the title ~~Charts: Dynamic x‑axis margin in LineChart to prevent crowded/clipped tick labels~~ Charts: Minimal dynamic X-axis margin in LineChart to fit tick labels Nov 17, 2025

kangzj approved these changes Nov 20, 2025

View reviewed changes

chriskmnds added 6 commits November 20, 2025 15:38

Charts: Dynamic x‑axis margin in LineChart to prevent crowded/clipped…

3ace9fd

… tick labels

changelog

47fc081

Charts: LineChart – minimal dynamic x-axis margin + documented margin…

5d30811

… defaults

Charts: simplify X-axis theme access in useChartMargin

e8f1d24

cleanup

cbcd541

Charts: add tests for dynamic X-axis margin behavior

a700d6b

kangzj force-pushed the update/fix-chart-x-axis-margins branch from 3666bcd to a700d6b Compare November 20, 2025 02:38

adamwoodnz added the [Status] Needs Review This PR is ready for review. label Nov 20, 2025

adamwoodnz added [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it 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 Nov 20, 2025

Charts: Minimal dynamic X-axis margin in LineChart to fit tick labels #45815

Are you sure you want to change the base?

Charts: Minimal dynamic X-axis margin in LineChart to fit tick labels #45815

Uh oh!

Conversation

chriskmnds commented Nov 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Proposed changes:

Other information:

Jetpack product discussion

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

Testing instructions:

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Code Coverage Summary

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chihsuan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chriskmnds commented Nov 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kangzj commented Nov 17, 2025

Uh oh!

chriskmnds commented Nov 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kangzj left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adamwoodnz commented Nov 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chriskmnds commented Nov 20, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

chriskmnds commented Nov 7, 2025 •

edited

Loading

github-actions bot commented Nov 7, 2025 •

edited

Loading

github-actions bot commented Nov 7, 2025 •

edited

Loading

jp-launch-control bot commented Nov 7, 2025 •

edited

Loading

chriskmnds commented Nov 10, 2025 •

edited

Loading

chriskmnds commented Nov 17, 2025 •

edited

Loading

kangzj left a comment •

edited

Loading

adamwoodnz commented Nov 20, 2025 •

edited

Loading