Skip to content

Premium Analytics: Use the Charts GeoDisplayMode type for the Locations map - #53016

Merged
Nikschavan merged 2 commits into
trunkfrom
update/charts-geo-display-mode-internal
Oct 2, 2026
Merged

Premium Analytics: Use the Charts GeoDisplayMode type for the Locations map#53016
Nikschavan merged 2 commits into
trunkfrom
update/charts-geo-display-mode-internal

Conversation

@Nikschavan

@Nikschavan Nikschavan commented Oct 1, 2026

Copy link
Copy Markdown
Member

Fixes N/A

Proposed changes

  • UNI-828: Premium Analytics: Show cities as markers on the Locations map #52859 added and exported a GeoDisplayMode type from Charts, but nothing used it: buildLocationsGeoChart wrote the same 'regions' | 'markers' union out by hand in its config type and in its cast. This follows up on the review comment there.
  • Re-export GeoDisplayMode through the Premium Analytics externals and use it in buildLocationsGeoChart, so the Locations map follows the Charts type if the display modes ever change. The resolution field keeps its own 'countries' | 'provinces' union, because GeoResolution also allows 'metros' and the Locations map reads the narrower type.

Related product discussion/links

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

No.

Testing instructions

  • Nothing to test by hand. This is a type-only change with no runtime effect, so the "Type checking" job and the Locations geo chart tests cover it.

@Nikschavan Nikschavan self-assigned this Oct 1, 2026
@Nikschavan
Nikschavan requested a review from louwie17 October 1, 2026 08:02
@Nikschavan
Nikschavan marked this pull request as ready for review October 1, 2026 08:03
@Nikschavan
Nikschavan requested a review from a team as a code owner October 1, 2026 08:03
Copilot AI balanced review requested due to automatic review settings October 1, 2026 08:03
@github-actions

github-actions Bot commented Oct 1, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.

  • To test on WoA, go to the Plugins menu on a WoA dev site. Click on the "Upload" button and follow the upgrade flow to be able to upload, install, and activate the Jetpack Beta plugin. Once the plugin is active, go to Jetpack > Jetpack Beta, select your plugin (Jetpack), and enable the update/charts-geo-display-mode-internal branch.
  • To test on Simple, run the following command on your sandbox:
bin/jetpack-downloader test jetpack update/charts-geo-display-mode-internal

Interested in more tips and information?

  • In your local development environment, use the jetpack rsync command to sync your changes to a WoA dev blog.
  • Read more about our development workflow here: PCYsg-eg0-p2
  • Figure out when your changes will be shipped to customers here: PCYsg-eg5-p2

@github-actions

github-actions Bot commented Oct 1, 2026

Copy link
Copy Markdown
Contributor

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 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:

  1. Ensure all required checks appearing at the bottom of this PR are passing.
  2. Make sure to test your changes on all platforms that it applies to. You're responsible for the quality of the code you ship.
  3. You can use GitHub's Reviewers functionality to request a review.
  4. 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!

@Nikschavan Nikschavan added [Status] Needs Review This PR is ready for review. and removed [Status] In Progress labels Oct 1, 2026
Copilot AI reviewed Oct 1, 2026
View reviewed changes

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

The type assertion bypasses the intended future compatibility check and should validate the scope object instead.

Review effort: Balanced
Findings: 1 Medium severity

Open (1)
What changed in this PR

Reuses Charts’ GeoDisplayMode type in Premium Analytics’ Locations map.

Changes:

  • Re-export GeoDisplayMode through Premium Analytics externals.
  • Apply it to the Locations chart configuration.
  • Add a type-only changelog entry.
File Description
build-geo-data.ts Uses GeoDisplayMode for map configuration.
externals/​src/​index.ts Re-exports the Charts type.
changelog/​update-pa-locations-geo-display-mode-type Records the internal type change.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread ...mium-analytics/packages/widgets-toolkit/src/components/locations-geo-chart/build-geo-data.ts Outdated
region: focusCountry && ! useCountryFallbackMap ? focusCountry.code.toUpperCase() : 'world',
resolution: ( useProvinceMap ? 'provinces' : 'countries' ) as 'countries' | 'provinces',
displayMode: ( mode === 'city' ? 'markers' : 'regions' ) as 'regions' | 'markers',
displayMode: ( mode === 'city' ? 'markers' : 'regions' ) as GeoDisplayMode,
@jp-launch-control

jp-launch-control Bot commented Oct 1, 2026 •
edited
Loading

Copy link
Copy Markdown

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 · PHP report · JS report

chihsuan
chihsuan approved these changes Oct 2, 2026
View reviewed changes

@chihsuan chihsuan left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! LGTM 🚢

@Nikschavan
refactor: Use GeoDisplayMode for the Locations map display mode
03e24e7 
Re-export the Charts GeoDisplayMode type through externals and use it in buildLocationsGeoChart in place of the inline 'regions' | 'markers' union.
@Nikschavan
fix: Check Locations map scope against its config type with satisfies
c994fcf 
A cast to GeoDisplayMode still compiles when Charts drops a display mode, because the literal union overlaps the narrower type. satisfies keeps the literals narrow and fails type checking instead.
@Nikschavan
Nikschavan force-pushed the update/charts-geo-display-mode-internal branch from e2f9662 to c994fcf Compare October 2, 2026 04:39
@Nikschavan
Nikschavan merged commit a3e778a into trunk Oct 2, 2026
81 checks passed
@Nikschavan
Nikschavan deleted the update/charts-geo-display-mode-internal branch October 2, 2026 05:35
@github-actions github-actions Bot removed the [Status] Needs Review This PR is ready for review. label Oct 2, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot
Copilot review effort
Applies to this pull request for everyone.Learn more about Copilot code review.
Copilot left review comments
@chihsuan chihsuan chihsuan approved these changes
@louwie17 louwie17 Awaiting requested review from louwie17

Assignees

@Nikschavan Nikschavan

Labels

[Package] Premium Analytics

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@Nikschavan @chihsuan