-
Notifications
You must be signed in to change notification settings - Fork 2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Stats Location: update geochart to use coordinates for Regions and Cities #99142
Conversation
Jetpack Cloud live (direct link)
Automattic for Agencies live (direct link)
|
This PR modifies the release build for the following Calypso Apps: For info about this notification, see here: PCYsg-OT6-p2
To test WordPress.com changes, run |
Here is how your PR affects size of JS and CSS bundles shipped to the user's browser: Sections (~281 bytes added 📈 [gzipped])
Sections contain code specific for a given set of routes. Is downloaded and parsed only when a particular route is navigated to. Legend What is parsed and gzip size?Parsed Size: Uncompressed size of the JS and CSS files. This much code needs to be parsed and stored in memory. Generated by performance advisor bot at iscalypsofastyet.com. |
…into fix/stats-locations-cities-map
@@ -209,22 +210,9 @@ const StatsLocations: React.FC< StatsModuleLocationsProps > = ( { query, summary | |||
</StatsInfoArea> | |||
); | |||
|
|||
const fakeData = [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We needed to add coordinates, and I think it’s too much data to keep in this file, so I preferred to move it to its own sample-locations.js file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work, Rafa! I'm approving it, as it LGTM, and we can roll it out as soon as we get support for Regions coordinates. ✅
Ensure that switching to the Cities tab renders the markers quickly.
Not just quickly, but instantly with no network requests:

Try to see the Cities tab on the summary/detailed page, and try some country filters.

The regions tab should not draw any marker.

Using a site with WordPress.com Personal plan, you can validate the Regions and Cities tabs are gated and loads markers in the blurry map.

As discussed in sync, we will proceed with merging this PR even if the coordinates for regions are not ready yet. However, since this PR enables support for them in Stats, we shouldn’t need to create another PR once they are ready. If we decide to pivot to a different approach, we will prepare a new PR accordingly. |
…ties (#99142) * Stats: add subdivisions tooltip * Update geochart to accept coordinates for Cities view * Omit coordinates property when no data exists * Handle region and city mode as markers * Filter locations with coordinates * Remove prop that is not needed for now * Refactor sample locations using coordinates * Simplify conditions * Remove unused attribute to sample locations --------- Co-authored-by: Tiago Ilieve <[email protected]>
Related to https://github.com/Automattic/jetpack-roadmap/issues/2242
Proposed Changes
Note
The regions API doesn’t return coordinates yet, but the idea is to have the code ready for when they become available.
Why are these changes being made?
We tried an approach that uses geocoding to obtain coordinates from the Google Maps API. The goal is to use the coordinates available in our Locations API to avoid relying on the Google Maps API.
Testing Instructions
Pre-merge Checklist