-
Notifications
You must be signed in to change notification settings - Fork 2k
Stats: Update the stats map in Odyssey stats to match the map in Calypso #103822
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
Conversation
Jetpack Cloud live (direct link)
Automattic for Agencies live (direct link)
|
Here is how your PR affects size of JS and CSS bundles shipped to the user's browser: Sections (~205 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. |
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.
Pull Request Overview
Fetch the user’s country code via a geolocation hook as a fallback when getCurrentUserCountryCode()
returns nothing, so the Stats geo map on Odyssey matches Calypso.
- Import and use
useGeoLocationQuery
anduseSelector
to retrieve a fallback country code. - Wrap the existing
StatsGeochart
inStatsGeochartWrapper
, computingfinalCountryCode
from user or geo data. - Change the default export to the new wrapper component.
Comments suppressed due to low confidence (2)
client/my-sites/stats/geochart/index.jsx:503
- [nitpick] The variable name
geoLocationReady
suggests a boolean flag but actually holds a country code string orfalse
. Consider renaming it tofallbackCountryCode
for clarity.
const geoLocationReady = ! isGeoLocationLoading && geoCountryCode;
client/my-sites/stats/geochart/index.jsx:497
- The new fallback logic via
useGeoLocationQuery
isn’t covered by existing tests. Adding tests for scenarios whencurrentUserCountryCode
is missing will ensure correct behavior.
const StatsGeochartWrapper = ( props ) => {
@@ -491,3 +493,17 @@ export default connect( ( state, ownProps ) => { | |||
statType, | |||
}; | |||
} )( localize( StatsGeochart ) ); | |||
|
|||
const StatsGeochartWrapper = ( props ) => { |
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.
[nitpick] StatsGeochartWrapper
isn’t documented with PropTypes or a displayName
. Adding PropTypes and setting StatsGeochartWrapper.displayName
would improve clarity in tooling and future maintenance.
Copilot uses AI. Check for mistakes.
Co-authored-by: Copilot <[email protected]>
This reverts commit cca0fa3.
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 |
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.
Good catch and nice fix! LGTM 👍
Part of #
Proposed Changes
useGeoLocationQuery()
hook to fetch the current user location as a fallback ifgetCurrentUserCountryCode()
does not return the country codegetCurrentUserCountryCode()
does not have the value as the current user is not set there and on Odyssey user's could not have a WordPress.com account evenWhy are these changes being made?
Testing Instructions
Before in Odyssey, when India is viewed from India
After
The map matches the Calypso map, where user country code is available from
getCurrentUserCountryCode()
Pre-merge Checklist