Charts: Add TimeSeriesForecastChart component#46844
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! |
There was a problem hiding this comment.
Pull request overview
This PR adds a new TimeSeriesForecastChart component to the charts package for visualizing time series data with forecast periods and uncertainty bands. The component provides a generic, type-safe way to display historical data alongside forecasted values with optional confidence intervals.
Changes:
- New chart component with support for historical and forecast data visualization
- Data transformation hook to split and process time series data
- Comprehensive Storybook stories demonstrating various use cases
- Package exports configuration for the new component
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| projects/js-packages/charts/src/index.ts | Exports the new TimeSeriesForecastChart component and its types |
| projects/js-packages/charts/src/charts/time-series-forecast-chart/types.ts | Type definitions for the component including props, accessors, and data structures |
| projects/js-packages/charts/src/charts/time-series-forecast-chart/time-series-forecast-chart.tsx | Main component implementation with responsive wrapper |
| projects/js-packages/charts/src/charts/time-series-forecast-chart/time-series-forecast-chart.module.scss | Component styling including tooltip, legend, and animation styles |
| projects/js-packages/charts/src/charts/time-series-forecast-chart/stories/index.stories.tsx | Storybook stories showcasing different configurations and use cases |
| projects/js-packages/charts/src/charts/time-series-forecast-chart/stories/config.tsx | Story configuration, sample data, and helper functions |
| projects/js-packages/charts/src/charts/time-series-forecast-chart/private/use-forecast-data.ts | Data transformation hook that splits data into historical/forecast portions |
| projects/js-packages/charts/src/charts/time-series-forecast-chart/private/index.ts | Internal exports for private utilities |
| projects/js-packages/charts/src/charts/time-series-forecast-chart/private/forecast-divider.tsx | Visual divider component marking the forecast start point |
| projects/js-packages/charts/src/charts/time-series-forecast-chart/index.ts | Public exports for the chart component and types |
| projects/js-packages/charts/package.json | Package exports configuration for the new component |
| projects/js-packages/charts/changelog/add-time-series-forecast-chart | Changelog entry documenting the new feature |
...ects/js-packages/charts/src/charts/time-series-forecast-chart/time-series-forecast-chart.tsx
Show resolved
Hide resolved
...packages/charts/src/charts/time-series-forecast-chart/time-series-forecast-chart.module.scss
Show resolved
Hide resolved
projects/js-packages/charts/src/charts/time-series-forecast-chart/private/use-forecast-data.ts
Outdated
Show resolved
Hide resolved
...ects/js-packages/charts/src/charts/time-series-forecast-chart/time-series-forecast-chart.tsx
Show resolved
Hide resolved
...ects/js-packages/charts/src/charts/time-series-forecast-chart/time-series-forecast-chart.tsx
Outdated
Show resolved
Hide resolved
projects/js-packages/charts/src/charts/time-series-forecast-chart/private/forecast-divider.tsx
Outdated
Show resolved
Hide resolved
...ects/js-packages/charts/src/charts/time-series-forecast-chart/time-series-forecast-chart.tsx
Outdated
Show resolved
Hide resolved
Code Coverage Summary6 files are newly checked for coverage. Only the first 5 are listed here.
Full summary · PHP report · JS report Coverage check overridden by
Coverage tests to be added later
|
767b628 to
6c926a5
Compare
projects/js-packages/charts/src/charts/time-series-forecast-chart/private/use-forecast-data.ts
Outdated
Show resolved
Hide resolved
...packages/charts/src/charts/time-series-forecast-chart/time-series-forecast-chart.module.scss
Outdated
Show resolved
Hide resolved
projects/js-packages/charts/src/charts/time-series-forecast-chart/private/use-forecast-data.ts
Outdated
Show resolved
Hide resolved
projects/js-packages/charts/src/charts/time-series-forecast-chart/private/use-forecast-data.ts
Show resolved
Hide resolved
bc9caef to
5ec636d
Compare
|
Thanks so much for taking a look @adamwoodnz ! I have addressed your comments inline.
Yep totally. I think this PR (and the followup) are just a bit big, so creating another PR right now to generate docs + API reference. That PR is here: #47152 |
| const { color: bandColor } = getElementStyles( { | ||
| index: 1, | ||
| data: { group: 'primary', label: seriesKeys.band, data: [] }, | ||
| } ); | ||
| const { lineStyles: forecastLineStyles, shapeStyles: forecastShapeStyles } = getElementStyles( { | ||
| index: 1, | ||
| data: { group: 'primary', label: seriesKeys.forecast, data: [], options: {} }, | ||
| } ); |
There was a problem hiding this comment.
I think these could be combined? Not sure if the label makes any difference...
| const { color: bandColor } = getElementStyles( { | |
| index: 1, | |
| data: { group: 'primary', label: seriesKeys.band, data: [] }, | |
| } ); | |
| const { lineStyles: forecastLineStyles, shapeStyles: forecastShapeStyles } = getElementStyles( { | |
| index: 1, | |
| data: { group: 'primary', label: seriesKeys.forecast, data: [], options: {} }, | |
| } ); | |
| const { color: bandColor, lineStyles: forecastLineStyles, shapeStyles: forecastShapeStyles } = getElementStyles( { | |
| index: 1, | |
| data: { group: 'primary', label: seriesKeys.band, data: [] }, | |
| } ); |
| // Create mock series data for theme hook | ||
| const mockSeriesData = useMemo( | ||
| () => [ | ||
| { label: seriesKeys.historical, data: [] }, | ||
| { label: seriesKeys.forecast, data: [] }, | ||
| ], | ||
| [ seriesKeys ] | ||
| ); | ||
| const theme = useXYChartTheme( mockSeriesData ); |
There was a problem hiding this comment.
?
Code smell - why do we need a mock in production code?
| export const CustomYDomain: StoryObj< StoryArgs > = Template.bind( {} ); | ||
| CustomYDomain.args = { | ||
| ...timeSeriesForecastChartStoryArgs, | ||
| yDomain: [ 0, 200 ], | ||
| }; | ||
|
|
||
| CustomYDomain.parameters = { | ||
| docs: { | ||
| description: { | ||
| story: 'Override the auto-calculated Y domain with a fixed range using the yDomain prop.', | ||
| }, | ||
| }, | ||
| }; |
There was a problem hiding this comment.
This story displays exactly the same as the Default story. I see the instruction to alter the yDomain prop, but I think it should be set differently from Default, otherwise it's not clear what effect it has.
| story: 'Different grid visibility options: y (default), x, xy, or none.', | ||
| }, | ||
| }, | ||
| }, |
| 'The chart supports generic data types via accessor functions. ' + | ||
| 'This example uses a custom datum shape with timestamp (number) instead of Date, ' + | ||
| 'and differently named fields for values and bounds.', |
There was a problem hiding this comment.
Why is this being concatenated?
| /** | ||
| * Legend at Top | ||
| */ | ||
| export const LegendAtTop: StoryObj< StoryArgs > = Template.bind( {} ); |
| } | ||
|
|
||
| svg { | ||
| overflow: visible; |
| /** | ||
| * Main component props (generic over datum type D) | ||
| */ | ||
| export interface TimeSeriesForecastChartProps< D > { |
There was a problem hiding this comment.
I think this should extend BaseChartProps like other charts
| const providerTheme = useGlobalChartsTheme(); | ||
| const { getElementStyles } = useGlobalChartsContext(); |
There was a problem hiding this comment.
| const providerTheme = useGlobalChartsTheme(); | |
| const { getElementStyles } = useGlobalChartsContext(); | |
| const { getElementStyles, theme: providerTheme } = useGlobalChartsContext(); |
adamwoodnz
left a comment
There was a problem hiding this comment.
Second pass done 🙂
A bit of cleanup to do. The only major is the resizing behaviour still isn't right.
|
Noting that we have a shared ChartLayout component for charts which support legends now, see #47554 |








Proposed changes:
TimeSeriesForecastChartcomponent for visualizing time series data with forecast periodsuseForecastDatahook for splitting data into historical and forecast segmentsForecastDividercomponent to visually separate historical from forecast dataOther information:
Does this pull request change what data or activity we track or use?
No, it does not.
Testing instructions:
add/time-series-forecast-chartbranch/jetpack/projects/js-packages/storybookpnpm run storybook:devforecastStartDatevalues to ensure proper data splitting