Charts: Small type, style and exports changes #41523

kangzj · 2025-02-03T22:46:26Z

Fixes #

Proposed changes:

Multiple small type, style and export fixes discovered when putting together Stats: Add line chart for real-time stats prototype wp-calypso#99141

Other information:

Have you written new tests for your changes, if applicable?
Have you checked the E2E test CI results, and verified that your changes do not break them?
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

n/a

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

no

Testing instructions:

Ensure all checks pass
Ensure changes look reasonable

github-actions · 2025-02-03T22:47:23Z

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 Team 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 🤖

The e2e test report can be found here. Please note that it can take a few minutes after the e2e tests checks are complete for the report to be available.

Follow this PR Review Process:

Ensure all required checks appearing at the bottom of this PR are passing.
Choose a review path based on your changes:
- A. Team Review: add the "[Status] Needs Team Review" label
  - For most changes, including minor cross-team impacts.
  - Example: Updating a team-specific component or a small change to a shared library.
- B. Crew Review: add the "[Status] Needs Review" label
  - For significant changes to core functionality.
  - Example: Major updates to a shared library or complex features.
- C. Both: Start with Team, then request Crew
  - For complex changes or when you need extra confidence.
  - Example: Refactor affecting multiple systems.
Get at least one approval before merging.

Still unsure? Reach out in #jetpack-developers for guidance!

kangzj · 2025-02-03T22:48:36Z

projects/js-packages/charts/src/components/line-chart/line-chart.tsx

@@ -157,7 +156,7 @@ const LineChart: FC< LineChartProps > = ( {
 				theme={ theme }
 				width={ width }
 				height={ height }
-				margin={ margin }
+				margin={ { top: 0, right: 0, bottom: 0, left: 0, ...margin } }


Default no margin, and this is also required for type checks.

kangzj · 2025-02-03T22:48:49Z

projects/js-packages/charts/src/components/pie-chart/pie-chart.tsx

@@ -20,11 +20,6 @@ interface PieChartProps extends OmitBaseChartProps {
 	 */
 	innerRadius?: number;

-	/**


Moved to base type

kangzj · 2025-02-03T22:49:04Z

projects/js-packages/charts/src/components/shared/with-responsive.tsx

@@ -23,7 +23,7 @@ export function withResponsive< T extends BaseChartProps< unknown > >(
 ) {
 	const { maxWidth = 1200, aspectRatio = 0.5, debounceTime = 50 } = config || {};

-	return function ResponsiveChart( props: Omit< T, 'width' | 'height' | 'size' > ) {
+	return function ResponsiveChart( props: Optional< T, 'width' | 'height' | 'size' > ) {


Fix the type checks from consumers

github-actions · 2025-02-03T22:49:25Z

Code Coverage Summary

This 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. 🤷

Full summary · JS report

kangzj · 2025-02-03T22:49:40Z

projects/js-packages/charts/src/components/shared/with-responsive.tsx

@@ -32,14 +32,13 @@ export function withResponsive< T extends BaseChartProps< unknown > >(

 		// Calculate dimensions
 		const containerWidth = parentWidth ? Math.min( parentWidth, maxWidth ) : 600;
-		const containerHeight = containerWidth * aspectRatio;
+		const containerHeight = props.height ?? containerWidth * aspectRatio;


Users should still be able to specify the height so that it'll be a different ratio even when responsive.

kangzj · 2025-02-03T22:49:56Z

projects/js-packages/charts/src/components/shared/with-responsive.tsx

@@ -23,7 +23,7 @@ export function withResponsive< T extends BaseChartProps< unknown > >(
 ) {
 	const { maxWidth = 1200, aspectRatio = 0.5, debounceTime = 50 } = config || {};


I'm wondering where can we change those params 🤔

What specifically are you interested in changing?

aspectRatio and maxWidth

kangzj · 2025-02-03T22:50:41Z

projects/js-packages/charts/src/index.ts

 export { ThemeProvider } from './providers/theme';
+export { defaultTheme, jetpackTheme, wooTheme } from './providers/theme/themes';


We need those exported which are required by ThemeProvider

kangzj · 2025-02-03T22:57:29Z

projects/js-packages/charts/src/components/shared/with-responsive.tsx


 		return (
 			<div
 				ref={ parentRef }
 				style={ {
 					width: '100%',
-					minHeight: `${ containerHeight }px`,


It otherwise set a min height for the wrapper which creates a big gap when the height is specified.

annacmc

This works well for me. All charts still work as expected, all tests pass 👍

kangzj added 3 commits February 4, 2025 11:39

a bunch small type and style fixes

e1101c9

changelog

be1a993

export themes

ee86c3f

kangzj added [Type] Janitorial [Status] Needs Team Review Obsolete. Use Needs Review instead. labels Feb 3, 2025

kangzj requested review from annacmc and a team February 3, 2025 22:46

kangzj self-assigned this Feb 3, 2025

github-actions bot added [JS Package] Charts RNA labels Feb 3, 2025

default no margin

e6af8a1

kangzj commented Feb 3, 2025

View reviewed changes

seems value is enough

d125547

kangzj commented Feb 3, 2025

View reviewed changes

annacmc approved these changes Feb 3, 2025

View reviewed changes

kangzj merged commit b160249 into trunk Feb 4, 2025
59 checks passed

kangzj deleted the update/type-and-style-fixes branch February 4, 2025 00:05

github-actions bot removed the [Status] Needs Team Review Obsolete. Use Needs Review instead. label Feb 4, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Charts: Small type, style and exports changes #41523

Charts: Small type, style and exports changes #41523

kangzj commented Feb 3, 2025 •

edited

Loading

github-actions bot commented Feb 3, 2025

kangzj Feb 3, 2025

kangzj Feb 3, 2025

kangzj Feb 3, 2025

github-actions bot commented Feb 3, 2025 •

edited

Loading

kangzj Feb 3, 2025

kangzj Feb 3, 2025

annacmc Feb 3, 2025 •

edited

Loading

kangzj Feb 4, 2025 •

edited

Loading

kangzj Feb 3, 2025

kangzj Feb 3, 2025 •

edited

Loading

annacmc left a comment

		@@ -23,7 +23,7 @@ export function withResponsive< T extends BaseChartProps< unknown > >(
		) {
		const { maxWidth = 1200, aspectRatio = 0.5, debounceTime = 50 } = config \|\| {};

		export { ThemeProvider } from './providers/theme';
		export { defaultTheme, jetpackTheme, wooTheme } from './providers/theme/themes';

Charts: Small type, style and exports changes #41523

Charts: Small type, style and exports changes #41523

Conversation

kangzj commented Feb 3, 2025 • edited Loading

Proposed changes:

Other information:

Jetpack product discussion

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

Testing instructions:

github-actions bot commented Feb 3, 2025

kangzj Feb 3, 2025

Choose a reason for hiding this comment

kangzj Feb 3, 2025

Choose a reason for hiding this comment

kangzj Feb 3, 2025

Choose a reason for hiding this comment

github-actions bot commented Feb 3, 2025 • edited Loading

Code Coverage Summary

kangzj Feb 3, 2025

Choose a reason for hiding this comment

kangzj Feb 3, 2025

Choose a reason for hiding this comment

annacmc Feb 3, 2025 • edited Loading

Choose a reason for hiding this comment

kangzj Feb 4, 2025 • edited Loading

Choose a reason for hiding this comment

kangzj Feb 3, 2025

Choose a reason for hiding this comment

kangzj Feb 3, 2025 • edited Loading

Choose a reason for hiding this comment

annacmc left a comment

Choose a reason for hiding this comment

kangzj commented Feb 3, 2025 •

edited

Loading

github-actions bot commented Feb 3, 2025 •

edited

Loading

annacmc Feb 3, 2025 •

edited

Loading

kangzj Feb 4, 2025 •

edited

Loading

kangzj Feb 3, 2025 •

edited

Loading