Skip to content

i18n: number-formatters – handle callback in geolocateCurrencySymbol #102538

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.

Sign up for GitHub

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

Jump to bottom
Merged
chriskmnds merged 2 commits into from
Apr 30, 2025
Merged

i18n: number-formatters – handle callback in geolocateCurrencySymbol #102538

chriskmnds merged 2 commits into from
Apr 30, 2025

Conversation

@chriskmnds
Copy link
Contributor

@chriskmnds chriskmnds commented Apr 10, 2025 •
edited
Loading

See PT: pxLjZ-9ME-p2
Part of addressing https://github.com/Automattic/i18n-issues/issues/942

Proposed Changes

  • Update geolocateCurrencySymbol in i18n-calypso to accept a callback. This is needed to set the geolocation in the number-formatters singleton used throughout the app.
  • Refactors purchases page to use @automattic/number-formatters for the billing description (as example use to confirm)

Alternatives:

  1. Populate the singleton directly from i18n-calypso
  2. Migrate geolocateCurrencySymbol out of i18n-calypso
  3. Return a Promise
  4. Reap it all out and use a Query hook (or existing hook)
  5. More holistic refactor

For the moment, passing a callback should suffice. I am not sure about importing number-formatters in i18n-calypso right now.

Media

Outside US:

Screenshot 2025-04-10 at 6 04 40 PM

From US:

Screenshot 2025-04-10 at 6 29 01 PM

Why are these changes being made?

Part of addressing https://github.com/Automattic/i18n-issues/issues/942
We need to ensure geo-location propagates to the number-formatters instance

Testing Instructions

  • Purchase any plan and go to /purchases/subscriptions/:site
  • If outside the US, the price should render with US prefix, otherwise without (per media above)
    • Can switch location with AutoProxxy: PCYsg-qMt-p2 (use atl for a US server)

Pre-merge Checklist

  • Has the general commit checklist been followed? (PCYsg-hS-p2)
  • Have you written new tests for your changes?
  • Have you tested the feature in Simple (P9HQHe-k8-p2), Atomic (P9HQHe-jW-p2), and self-hosted Jetpack sites (PCYsg-g6b-p2)?
  • Have you checked for TypeScript, React or other console errors?
  • Have you used memoizing on expensive computations? More info in Memoizing with create-selector and Using memoizing selectors and Our Approach to Data
  • Have we added the "[Status] String Freeze" label as soon as any new strings were ready for translation (p4TIVU-5Jq-p2)?
    • For UI changes, have we tested the change in various languages (for example, ES, PT, FR, or DE)? The length of text and words vary significantly between languages.
  • For changes affecting Jetpack: Have we added the "[Status] Needs Privacy Updates" label if this pull request changes what data or activity we track or use (p4TIVU-aUh-p2)?

@chriskmnds chriskmnds self-assigned this Apr 10, 2025
@chriskmnds chriskmnds requested review from a team as code owners April 10, 2025 15:31
@matticbot matticbot added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Apr 10, 2025
@chriskmnds chriskmnds requested review from Copilot and removed request for a team April 10, 2025 15:31
@github-actions
Copy link

github-actions bot commented Apr 10, 2025 •
edited by matticbot
Loading

Calypso Live (direct link)
https://calypso.live?image=registry.a8c.com/calypso/app:build-142188
Jetpack Cloud live (direct link)
https://calypso.live?image=registry.a8c.com/calypso/app:build-142188&env=jetpack
Automattic for Agencies live (direct link)
https://calypso.live?image=registry.a8c.com/calypso/app:build-142188&env=a8c-for-agencies

@chriskmnds chriskmnds requested a review from a team April 10, 2025 15:31
Copilot AI reviewed Apr 10, 2025
View reviewed changes
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.

Comments suppressed due to low confidence (1)

client/boot/common.js:10

  • The import path 'path-to-number-formatters' is inconsistent with other files that import setGeoLocation from '@automattic/number-formatters'. Consider updating the import to ensure consistency.
import { setGeoLocation } from 'path-to-number-formatters';

@matticbot
Copy link
Contributor

matticbot commented Apr 10, 2025 •
edited
Loading

Here is how your PR affects size of JS and CSS bundles shipped to the user's browser:

App Entrypoints (~75 bytes added 📈 [gzipped])

Details 
name                   parsed_size           gzip_size
entry-stepper               +119 B  (+0.0%)      +41 B  (+0.0%)
entry-main                  +119 B  (+0.0%)      +47 B  (+0.0%)
entry-subscriptions          +41 B  (+0.0%)      +13 B  (+0.0%)
entry-reauth-required        +41 B  (+0.0%)      +13 B  (+0.0%)
entry-login                  +41 B  (+0.0%)      +13 B  (+0.0%)
entry-domains-landing        +23 B  (+0.0%)       +4 B  (+0.0%)

Common code that is always downloaded and parsed every time the app is loaded, no matter which route is used.

Sections (~13 bytes added 📈 [gzipped])

Details 
name            parsed_size           gzip_size
site-purchases        +26 B  (+0.0%)      +13 B  (+0.0%)
purchases             +26 B  (+0.0%)      +13 B  (+0.0%)

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.
Gzip Size: Compressed size of the JS and CSS files. This much data needs to be downloaded over network.

Generated by performance advisor bot at iscalypsofastyet.com.

@matticbot
Copy link
Contributor

matticbot commented Apr 10, 2025

This PR modifies the release build for the following Calypso Apps:

For info about this notification, see here: PCYsg-OT6-p2

  • blaze-dashboard
  • command-palette-wp-admin
  • happy-blocks
  • help-center
  • notifications
  • odyssey-stats
  • wpcom-block-editor

To test WordPress.com changes, run install-plugin.sh $pluginSlug update/i18n-number-formatters-geolocate-callback on your sandbox.

@chriskmnds chriskmnds force-pushed the update/i18n-number-formatters-init branch from dfabeaf to 99d4dff Compare April 11, 2025 08:51
@chriskmnds chriskmnds mentioned this pull request Apr 25, 2025
Merged
8 tasks
@chriskmnds chriskmnds force-pushed the update/i18n-number-formatters-init branch from e7e58ca to 87e73b1 Compare April 25, 2025 09:06
@chriskmnds chriskmnds force-pushed the update/i18n-number-formatters-geolocate-callback branch from e491948 to 97149e1 Compare April 25, 2025 09:10
@chriskmnds chriskmnds mentioned this pull request Apr 25, 2025
Merged
@chriskmnds chriskmnds force-pushed the update/i18n-number-formatters-init branch from 642a186 to ac7cc06 Compare April 30, 2025 09:05
Base automatically changed from update/i18n-number-formatters-init to trunk April 30, 2025 09:36
@chriskmnds chriskmnds force-pushed the update/i18n-number-formatters-geolocate-callback branch from 97149e1 to 4fac53c Compare April 30, 2025 09:40
@chriskmnds chriskmnds merged commit 0ca04d5 into trunk Apr 30, 2025
15 checks passed
@chriskmnds chriskmnds deleted the update/i18n-number-formatters-geolocate-callback branch April 30, 2025 12:12
@github-actions github-actions bot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Apr 30, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot Copilot left review comments

Assignees

@chriskmnds chriskmnds

Labels

i18n: number-format

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@chriskmnds @matticbot