Charts: Add Woo Analytics Leaderboard chart component #44299
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?
|
github-actions
bot
added
[JS Package] Charts
[Plugin] Backup
|
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! Jetpack plugin: The Jetpack plugin has different release cadences depending on the platform:
If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. Backup plugin: No scheduled milestone found for this plugin. If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. Boost plugin: No scheduled milestone found for this plugin. If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. Search plugin: No scheduled milestone found for this plugin. If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. Social plugin: No scheduled milestone found for this plugin. If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. Starter Plugin plugin: No scheduled milestone found for this plugin. If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. Protect plugin:
If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. Videopress plugin: No scheduled milestone found for this plugin. If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. |
github-actions
bot
added
the
[Status] Needs Author Reply
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a new LeaderboardChart component for Woo Analytics, complete with data utilities, formatting, tests, Storybook stories, and documentation.
- Adds
LeaderboardChart, its utilities (buildLeaderboardData,calculateDelta,formatMetricValue), and related types to the charts package. - Implements comprehensive tests, sample data, and Storybook stories for the new component.
- Updates individual plugin changelogs to note inclusion of the new chart component.
Reviewed Changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| projects/plugins/*/changelog/update-add_woo_leaderboard_chart_component | Changelog entries for each plugin |
| projects/js-packages/charts/src/index.ts | Export new component, utilities, and types |
| projects/js-packages/charts/src/components/leaderboard-chart/utils.ts | Data processing helpers (calculateDelta, etc.) |
| projects/js-packages/charts/src/components/leaderboard-chart/leaderboard-chart.tsx | Core component implementation and JSDoc |
| projects/js-packages/charts/src/components/leaderboard-chart/test/leaderboard-chart.test.tsx | Jest tests for component and utils |
| projects/js-packages/charts/src/components/leaderboard-chart/format-metric-value.ts | Number formatting utility |
| projects/js-packages/charts/src/components/leaderboard-chart/README.md | Component documentation and usage guide |
| projects/js-packages/charts/changelog/update-add_woo_leaderboard_chart_component | Main changelog entry for charts package |
| projects/js-packages/charts/LEADERBOARD_INTEGRATION.md | Integration guide for consuming applications |
Comments suppressed due to low confidence (3)
projects/js-packages/charts/src/components/leaderboard-chart/leaderboard-chart.tsx:28
- The JSDoc for
currentSharestates “% of the current value,” but the code computes it relative to the maximum value across all entries. Update the comment to clarify it’s “% of the max data value.”
* Width of current bar, as % of the current value
projects/js-packages/charts/src/components/leaderboard-chart/leaderboard-chart.tsx:33
- Similar to
currentShare, thepreviousSharecomment should reflect that it’s computed as a percentage of the max data value, not the current value.
* Width of previous bar, as % of the current value
projects/js-packages/charts/src/components/leaderboard-chart/test/leaderboard-chart.test.tsx:104
- This test only checks that entries are absent for empty data. It should also assert that the empty state message ("No data available") is rendered.
it( 'handles empty data', () => {
projects/plugins/jetpack/changelog/update-add_woo_leaderboard_chart_component
Outdated
Show resolved
Hide resolved
Code Coverage Summary3 files are newly checked for coverage.
|
dognose24
added
[Type] Feature
dognose24
force-pushed
the
update/add_woo_leaderboard_chart_component
branch
from
c01035e to
911fa7a
Compare
dognose24
force-pushed
the
update/add_woo_leaderboard_chart_component
branch
from
faf0782 to
1f5c35f
Compare
|
Thanks for the review, @chihsuan! 🙏🏼 I updated the PR, which could use another review round. By the way, would you have preferred to use core components like |
| const deltaColor = signColors[ colorIndex ]; | ||
|
|
||
| return ( | ||
| <Fragment key={ entry.id }> |
There was a problem hiding this comment.
Do we need this Fragment instance wrapping the div here? I think we can remove it.
There was a problem hiding this comment.
That makes sense, we can move the key attribute to the next level. Updated in 41dcbc5.
| width: 100%; | ||
| max-width: 400px; | ||
| display: flex; | ||
| flex-direction: column; | ||
| gap: 12px; |
There was a problem hiding this comment.
If we are going to begin using HTML+CSS to compose the chart (as we do here), we should also consider using WordPress components. The regular div with these styles could be replaced with the <VStack /> component, for instance.
I'd like to know the opinion of other Chart development folks here, though.
There was a problem hiding this comment.
I agree with Damián.
I noticed that the Chart package doesn't currently use any WordPress components, perhaps we should check with @kangzj to confirm.
There was a problem hiding this comment.
The regular div with these styles could be replaced with the component, for instance.
Yes, I struggled with that, but I turned the draft into a PR for more solid feedback. We should reuse the core component as much as possible to avoid design deviations.
I noticed that the Chart package doesn't currently use any WordPress components
That might deserve a confirmation about whether we plan to introduce more bundle sizes for the library. 🙂
There was a problem hiding this comment.
We should probably try to use as less dependencies as possible; however Core components are used in almost all of our projects, and if consumers are not using HTML based charts, they could be shook off by their bundlers, so I think it's fine to add it.
There was a problem hiding this comment.
Sounds thoughtful and reasonable! 💯
| /** | ||
| * Value of the entry | ||
| */ | ||
| currentValue: number; | ||
|
|
||
| /** | ||
| * Value of the entry in the previous period | ||
| */ | ||
| previousValue: number; |
There was a problem hiding this comment.
The implementation is limited to handling only two entries per item. Is that correct
Do you think we should consider handling more than two entries?
This is an example of a BarChart rendering three entries per item, for instance.
There was a problem hiding this comment.
Yeah, it's the current implementation. I was aiming first to migrate the component to allow for more variation in the future.
Do we already have use cases or a design for the leaderboard chart with more than two series?
There was a problem hiding this comment.
I think we should keep it as a migration PR but not one to add more functionality. We could look into this later.
There was a problem hiding this comment.
Thansk, @kangzj! I agreed with it. 🙂
There was a problem hiding this comment.
Thanks @dognose24 for this work. My feedback/suggestions.
I think we should iterate on the Chart API to make it simpler and more flexible. Several of the current props (delta, share, etc.) are derived values that the chart can compute internally based on the input data. Moving that logic inside the chart would help keep the data shape minimal and semantic.
Another point is the assumption of having exactly two data points per entry (current and previous). That limits the use case. Ideally, the chart should support rendering two, three, or more values per item. For example, to compare multiple time periods or targets.
Here's a rough example of how the data could look with three samples per entry:
{
id: 'google',
label: 'Google',
series: [
{ key: '2024', label: 'YTD 2024', value: 324000 },
{ key: '2023', label: 'YTD 2023', value: 296000 },
{ key: 'target', label: 'Target 2024', value: 400000 },
]
}And a possible type definition:
type LeaderboardSeries = {
key: string; // unique identifier, used for color mapping or legends
label?: string; // human-friendly label
value: number;
};
type LeaderboardEntry = {
id: string; // unique identifier per row/item
label: string; // display label for the row
series: LeaderboardSeries[];
};I know this implementation is based on the Analytics one, but I think this chart could benefit from a more general and scalable data structure.
Please take it as a suggestion 🙇♂️
|
@retrofox Thanks for the feedback! 👍🏼
Indeed, we just need to restore the
The data structure would be extensible for multiple items in a row. However, introducing more than two items in a row would mean the display format might differ from the current one we use in Analytics. We might need to show each value for every progress bar inside the row and remove the delta part, wouldn't we? If we want multiple bars without comparison, the data could be independent for each row, like 
Does it make sense? |
There was a problem hiding this comment.
Nice docs! Could we consider integrating these into Storybook? There is an approach discussed on CHARTS-44: Improve Storybook Docs
There was a problem hiding this comment.
Integrated in 2a55a2a.
I have included 
|
|
|
||
| ### Basic Usage | ||
|
|
||
| \`\`\`typescript |
There was a problem hiding this comment.
Do we need to scape the backslash chars? Is it a trick to not break the markdown format in the AI app? 😅
Ignore me please :-)
| className, | ||
| style, | ||
| } ) => { | ||
| const signColors = [ '#D63638', '#757575', '#008A20' ]; |
There was a problem hiding this comment.
Should we try to handle this from the ThemeProvider component? If so, I think we can add a ToDo block for now.
There was a problem hiding this comment.
Added in 7ee47b7.
github-actions
bot
removed
the
[Status] Needs Review
Related to CHARTS-34.
Proposed changes:
LeaderboardChartcomponent from the Woo Analytics repo to the Chart Library for reuse.Other information:
Jetpack product discussion
pejTkB-27k-p2.
Does this pull request change what data or activity we track or use?
Testing instructions:
cd /path-to/jetpack/projects/js-packages/storybook && pnpm run storybook:dev.