CHARTS-15 - Add support to enable default start glyph for the line chart #43819

Nikschavan · 2025-06-06T11:58:15Z

Fixes CHARTS-15

Proposed changes:

Add a prop to enable/disable the glyph at the start of the line in line chart

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

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

No

Testing instructions:

Start the storybook per steps in the charts library - https://github.com/Automattic/jetpack/tree/27d5924896c6c4cfabded89fa1b6247aaefd2e55/projects/js-packages/charts#storybook
Test the story "With Start Glyphs"

github-actions · 2025-06-06T11:59:13Z

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-06-06T12:00:56Z

Code Coverage Summary

Coverage changed in 1 file.

File	Coverage	Δ%	Δ Uncovered
projects/js-packages/charts/src/components/line-chart/line-chart.tsx	53/65 (81.54%)	5.54%	0 💚

Full summary · JS report

Nikschavan · 2025-06-06T12:06:44Z

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

+									data={ seriesData }
+									color={ stroke }
+									glyphStyle={ glyphStyle }
+									renderStartGlyph={ defaultRenderGlyph }


Right now, I am not allowing overriding the glyph and using the default style, which matches the style of glyphs shown with the tooltip/hover.

I assume in the separate task we will probably want to allow using custom glyphs per data? pcO4xN-2zR-p2

Copilot

Pull Request Overview

Adds support for rendering a glyph at the start of each series in the line chart when enabled

Introduces a withStartGlyphs prop and default glyph renderer
Implements a StartGlyph component and exports its props type
Adds a Storybook story and changelog entry for the new feature

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

File	Description
projects/js-packages/charts/src/index.ts	Export `RenderLineStartGlyphProps` type
projects/js-packages/charts/src/components/line-chart/stories/index.stories.tsx	Add `WithStartGlyphs` story
projects/js-packages/charts/src/components/line-chart/line-chart.tsx	Add `withStartGlyphs` prop, `StartGlyph` component, and default glyph rendering
projects/js-packages/charts/changelog/charts-15-add-glyphs-at-the-start-of-the-lines	Add changelog for the new glyph feature

Comments suppressed due to low confidence (2)

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

The new withStartGlyphs prop should be documented in the JSDoc comment for LineChartProps, including its default value and behavior.

withStartGlyphs?: boolean;

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

The logic for rendering start glyphs isn't covered by unit tests. Consider adding tests to verify that when withStartGlyphs is true, the StartGlyph component is rendered correctly.

renderTooltip = renderDefaultTooltip,

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

Co-authored-by: Copilot <[email protected]>

…s://github.com/Automattic/jetpack into charts-15-add-glyphs-at-the-start-of-the-lines

dognose24

Works as described, nice work! 👍🏼 Left my thought, but it won't be a blocker. 🙂

dognose24 · 2025-06-06T15:05:37Z

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

+	data: SeriesData;
+	index: number;
+	color: string;
+	renderStartGlyph: < Datum extends object >(


I would put my 2 cents here: I think the renderStartGlyph function could be more converged inside the component StartGlyph rather than exposing the whole render function, since the primary purpose is to add the glyph at the start point of the line chart in this scenario. I imagine the StartGlyph component would receive a simple icon or element for a customizable glyph.

Is there a more complex use case to adjust more options for the glyph point?

I was imagining that if we were to allow changing the glyph and add a custom shape here, we would want to match the one on the tooltip. That's why I have kept the type/format of this same as the one from the ToolTip Glyph in visx, so that same Glyph could be potentially passed to Tooltip as well - https://github.com/airbnb/visx/blob/86738178e86fdc0feacf83ac96e68724da0627c2/packages/visx-xychart/src/components/Tooltip.tsx#L75-L102C17

so that same Glyph could be potentially passed to Tooltip as well

👍

kangzj

Looks good to me! Nice one 👍

… component

Nikschavan added 4 commits June 6, 2025 16:58

Charts: Add support to enable default start glyph for the line chart

c2a3c12

Don't allow overriding the default render glyph

1a50927

Add changelog entry

27d5924

Rename the component singluar name

6663ea2

github-actions bot added the [JS Package] Charts label Jun 6, 2025

github-actions bot added RNA [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. labels Jun 6, 2025

Nikschavan added the [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it label Jun 6, 2025

Nikschavan requested a review from Copilot June 6, 2025 12:04

Nikschavan commented Jun 6, 2025

View reviewed changes

This comment was marked as outdated.

Sign in to view

Nikschavan added 2 commits June 6, 2025 17:38

forward glyphstyle to default glyph style

97856b9

Don't force glyphStyle at a parent component level

30ecb98

Nikschavan requested a review from Copilot June 6, 2025 12:19

Nikschavan added the [Status] Needs Team Review Obsolete. Use Needs Review instead. label Jun 6, 2025

Nikschavan requested a review from a team June 6, 2025 12:20

Copilot AI reviewed Jun 6, 2025

View reviewed changes

Add tests for start glyphs

a77ed63

github-actions bot added the [Tests] Includes Tests label Jun 6, 2025

Nikschavan and others added 3 commits June 6, 2025 18:03

Remove redundand check for firstPoint

6974eb0

Co-authored-by: Copilot <[email protected]>

Add key to the DefaultGlyph

21fc5d7

Merge branch 'charts-15-add-glyphs-at-the-start-of-the-lines' of http…

c44b0a8

…s://github.com/Automattic/jetpack into charts-15-add-glyphs-at-the-start-of-the-lines

dognose24 previously approved these changes Jun 6, 2025

View reviewed changes

kangzj self-requested a review June 9, 2025 03:07

kangzj previously approved these changes Jun 9, 2025

View reviewed changes

Merge branch 'trunk' into charts-15-add-glyphs-at-the-start-of-the-lines

d84f017

Nikschavan dismissed stale reviews from kangzj and dognose24 via d84f017 June 9, 2025 06:52

Rename the prop renderGlyph as this is already part of the StartGlyph…

c3b7d2c

… component

Nikschavan merged commit c883c8f into trunk Jun 9, 2025
63 checks passed

Nikschavan deleted the charts-15-add-glyphs-at-the-start-of-the-lines branch June 9, 2025 07:41

github-actions bot removed [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. [Status] Needs Team Review Obsolete. Use Needs Review instead. labels Jun 9, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

CHARTS-15 - Add support to enable default start glyph for the line chart #43819

CHARTS-15 - Add support to enable default start glyph for the line chart #43819

Uh oh!

Nikschavan commented Jun 6, 2025 •

edited

Loading

Uh oh!

github-actions bot commented Jun 6, 2025 •

edited

Loading

Uh oh!

jp-launch-control bot commented Jun 6, 2025 •

edited

Loading

Uh oh!

Nikschavan Jun 6, 2025 •

edited

Loading

Uh oh!

This comment was marked as outdated.

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dognose24 left a comment

Uh oh!

dognose24 Jun 6, 2025

Uh oh!

Nikschavan Jun 7, 2025

Uh oh!

kangzj Jun 9, 2025

Uh oh!

kangzj left a comment

Uh oh!

Uh oh!

Uh oh!

CHARTS-15 - Add support to enable default start glyph for the line chart #43819

CHARTS-15 - Add support to enable default start glyph for the line chart #43819

Uh oh!

Conversation

Nikschavan commented Jun 6, 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 Jun 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Uh oh!

Code Coverage Summary

Uh oh!

Nikschavan Jun 6, 2025 • 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!

This comment was marked as outdated.

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dognose24 left a comment

Choose a reason for hiding this comment

Uh oh!

dognose24 Jun 6, 2025

Choose a reason for hiding this comment

Uh oh!

Nikschavan Jun 7, 2025

Choose a reason for hiding this comment

Uh oh!

kangzj Jun 9, 2025

Choose a reason for hiding this comment

Uh oh!

kangzj left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Nikschavan commented Jun 6, 2025 •

edited

Loading

github-actions bot commented Jun 6, 2025 •

edited

Loading

jp-launch-control bot commented Jun 6, 2025 •

edited

Loading

Nikschavan Jun 6, 2025 •

edited

Loading