Charts: Update GeoChart to support Google Charts native data format with custom tooltips #46330
Conversation
- Updated GeoData type to align with Google Charts format, allowing for flexible data representation. - Refactored GeoChartProps to accept data in the new format, supporting both full country names and ISO 3166-1 alpha-2 codes. - Improved documentation and examples to reflect the new data structure and advanced features like custom tooltips and formatted values. - Updated tests to validate the new data handling and tooltip functionalities.
|
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! |
github-actions
bot
added
the
[Status] Needs Author Reply
adamwoodnz
added
[Type] Enhancement
…redundant comments. Simplified the description of country identifiers while retaining essential information for users.
Code Coverage SummaryCoverage changed in 1 file.
|
…ted tooltip options to conditionally include HTML support based on data structure, improving user experience for interactive charts.
- Updated test to expect isHtml: false by default - Added test for HTML tooltip detection (isHtml: true) - Added test for text-only tooltips (isHtml: false) - Updated story column names for consistency (Value → Orders/Views)
adamwoodnz
added
[Status] Needs Review
There was a problem hiding this comment.
Pull request overview
This PR modernizes the GeoChart component to use Google Charts' native data format instead of a simplified custom format, enabling support for custom tooltips (both text and HTML), formatted values, and other advanced Google Charts features. The change is a breaking API change that improves flexibility and developer experience with comprehensive TypeScript support.
Key Changes:
- Updated
GeoDatatype fromRecord<string, number>to Google Charts native format[GoogleDataTableColumn[], ...GoogleDataTableRow[]] - Added support for custom HTML tooltips, text tooltips, and formatted values with automatic HTML tooltip detection
- Exported Google Charts types (
GoogleDataTableColumn,GoogleDataTableRow,GoogleDataTableColumnRoleType) directly from the package - Updated all examples, tests, and documentation to use full country names instead of ISO codes (though both are still supported)
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
src/types.ts |
Updated GeoData type to use Google Charts native format with comprehensive JSDoc examples |
src/stories/sample-data/index.ts |
Converted sample data from object format to array format with full country names |
src/index.ts |
Exported Google Charts types for consumer use |
src/charts/geo-chart/types.ts |
Updated prop documentation to reflect new data format |
src/charts/geo-chart/test/geo-chart.test.tsx |
Updated all tests for new data format and added tests for HTML tooltip detection |
src/charts/geo-chart/stories/index.stories.tsx |
Added comprehensive story examples showcasing custom tooltips, formatted values, and complex HTML tooltips |
src/charts/geo-chart/stories/index.docs.mdx |
Enhanced documentation with detailed examples of advanced features and full country name usage |
src/charts/geo-chart/stories/index.api.mdx |
Updated API reference with new type definitions and comprehensive examples |
src/charts/geo-chart/geo-chart.tsx |
Removed data transformation logic and added HTML tooltip detection to configure Google Charts options |
changelog/charts-150-add-support-for-custom-tooltip-rendering |
Added changelog entry documenting the breaking change |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
projects/js-packages/charts/src/charts/geo-chart/test/geo-chart.test.tsx
Show resolved
Hide resolved
projects/js-packages/charts/changelog/charts-150-add-support-for-custom-tooltip-rendering
Show resolved
Hide resolved
…yCountry for improved accuracy in visual representation.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
- Updated test for data passing to Google Charts to ensure it reflects complex data structures, including formatted values and tooltips. - Removed outdated tests that checked for simpler data formats, focusing on validating the integrity of the data as provided. - Improved clarity in test descriptions to better reflect the functionality being tested.
- Introduced useMemo for hasHtmlTooltips and options to enhance performance by memoizing values based on dependencies. - Improved readability and efficiency of the GeoChart component by reducing unnecessary recalculations.
| | Prop | Type | Default | Description | | ||
| | ---- | ---- | ------- | ----------- | | ||
| | `data` | `Record<string, number>` | - | **Required.** Record mapping country ISO 3166-1 alpha-2 codes (e.g., 'US', 'CA', 'GB') to numeric values | | ||
| | `data` | `GeoData` | - | **Required.** Data in Google Charts format. First row contains column headers, subsequent rows contain data. Countries can be identified by full name (e.g., 'United States') or ISO codes (e.g., 'US'). Full names are recommended. | |
There was a problem hiding this comment.
Full names are recommended
Is this correct? I took a look at https://developers.google.com/chart/interactive/docs/gallery/geochart#data-format and didn't find the narrative.
There was a problem hiding this comment.
Full names are recommended for display purposes, maybe the comment should be updated?
From that document:
A country name as a string (for example, "England"), or an uppercase ISO-3166-1 alpha-2 code or its English text equivalent (for example, "GB" or "United Kingdom").
There was a problem hiding this comment.
What happens for regions / states in this case? I imagined there would be a separate string for display 🤔
There was a problem hiding this comment.
Full names are recommended for display purposes
If the tooltip could be cusomized, then using country / region code doesn't feel like an issue or at least using full names shouldn't be recommended imho. Country names are longer and harder to use I think and sometimes might not be easy to be accurate especially if we want to do filtering etc later. And I think we wanted to add support for regions / states as well, which made it even harder.
There was a problem hiding this comment.
Here's the beginnings of the PR for states so you can see what will be involved: #46332
It's possible to use ISO codes and still display the country name as the tooltip title, but the full country name would also need to be included in the data like this:
There was a problem hiding this comment.
I see the Jetpack Stats data is like this, with both included:
[
{
"label": "Australia",
"countryCode": "AU",
"countryFull": "Australia",
"value": 14,
"region": "053"
},
{
"label": "New Zealand",
"countryCode": "NZ",
"countryFull": "New Zealand",
"value": 1,
"region": "053"
}
]
| data.length > 0 && | ||
| data[ 0 ].some( | ||
| col => | ||
| typeof col === 'object' && |
There was a problem hiding this comment.
Is it possible to use optional chaining to simply the conditions here?
There was a problem hiding this comment.
No unfortunately, due to TS. Claude's explanation:
The verbose version is required because GoogleDataTableColumn can be either a string or an object. While optional chaining (?.) helps with null/undefined checks, TypeScript still needs explicit type narrowing to distinguish between strings and objects in a union type.
The issue: The data format allows columns to be defined as either:
Simple strings: 'Country'
Objects with metadata: { type: 'string', role: 'tooltip', p: { html: true } }
Why optional chaining alone doesn't work: When TypeScript sees a union type (string | object), it can't safely assume properties exist even with ?. because strings don't have those properties at all. We need the typeof check and in operator to properly narrow the type.
This is the correct TypeScript pattern for type-safe property access on union types.
| legend: 'none', | ||
| keepAspectRatio: true, | ||
| }; | ||
| const options: GoogleChartOptions = useMemo( |
…t types to emphasize the use of full names for better tooltip readability.
kangzj
left a comment
kangzj
left a comment
There was a problem hiding this comment.
Happy to start with this and see what we can improve on 
Fixes CHARTS-150: Add support for custom tooltip rendering
Proposed changes:
This PR enhances the GeoChart component to use Google Charts native data format, providing maximum flexibility for consumers to customize tooltip content, formatted values, and more.
Key Changes:
GoogleDataTableColumn[],GoogleDataTableRow[]) instead of custom simplified typesGoogleDataTableColumn,GoogleDataTableRow,GoogleDataTableColumnRoleType) following the same pattern as visx typesScreenshots
Benefits:
Other information:
Jetpack product discussion
Does this pull request change what data or activity we track or use?
No, this PR only updates the data format for the GeoChart component. No tracking or data collection changes.
Testing instructions:
Basic Usage
projects/js-packages/chartspnpm installandpnpm storybookCustom Tooltips
Formatted Values
Text Tooltips
Complex Tooltips
TypeScript Usage
Verify Type Checking
cd projects/js-packages/charts pnpm typecheckAll types should compile without errors.