localize-currency-amount: Utility for locale-aware currency formatting #36758

nbloomf · 2019-10-15T04:34:08Z

Formatting monetary amounts depends on the currency and the user's locale. This package adds a function for dealing with this.

Changes proposed in this Pull Request

Adds localizeMonetaryAmount(). This improves on other methods by working with integers only and using non breaking spaces everywhere.

Testing instructions

npm run test-packages localize-monetary-amount

matticbot · 2019-10-15T04:34:10Z

Test live: https://calypso.live/?branch=add/wp-checkout-format-currency

matticbot · 2019-10-15T04:41:01Z

This PR does not affect the size of JS and CSS bundles shipped to the user's browser.

Generated by performance advisor bot at iscalypsofastyet.com.

sirbrillig

This looks amazing. I think we can actually make this into its own package and then depend on it.

packages/wp-checkout/src/lib/format-monetary-amount-for-locale.js

sirbrillig · 2019-10-18T15:07:53Z

I also just saw this, but I don't know much about it: https://github.com/Automattic/wp-calypso/tree/master/packages/format-currency

sirbrillig · 2019-10-21T21:39:02Z

I think this is great. My inclination is to wait until #36720 is merged and then make this PR into its own package (maybe @automattic/format-amount-for-locale?) and add it to the dependencies of @automattic/composite-component.

nbloomf · 2019-10-21T21:44:36Z

That sounds good to me.

This library was written for the in-progress composite-checkout package but has no external dependencies, so we're extracting it as its own package. Features: exact arithmetic and non-breaking spaces

packages/localize-monetary-amount/src/localize-monetary-amount.ts

sirreal

This is some impressive work, well done 💯

I did my best to provide a thorough review. I learned a lot in the process 🙂

A few high level notes:

We should copy .eslintrc from a sibling package. That's required for one of our package dependency lint rules.
We need to get the build set up and I'd like to use the tsc compiler to output types.
Let's remove types from jsdoc, they're redundant.
We need to check browser compatibility. If we're targetting es5, we should use es5 or make sure that code is properly transpiled. I don't know whether TypeScript handles that.
The structure and comments are impressive and make this pretty easy to follow even when it's completely new material. Well done 👍

packages/localize-monetary-amount/src/currency-code.ts

packages/localize-monetary-amount/src/english-currency-name.ts

packages/localize-monetary-amount/src/localize-monetary-amount.ts

sirreal · 2019-11-07T22:47:39Z

packages/localize-monetary-amount/src/localize-monetary-amount.ts

+	currencyCode: CurrencyCode,
+	absoluteAmount: number
+): { integerPart: string[]; fractionalPart: string } {
+	if ( ! ( Number.isInteger( absoluteAmount ) && absoluteAmount >= 0 ) ) {


Flagging Number.isInteger again (see my other comment)

sirreal · 2019-11-07T22:51:28Z

packages/localize-monetary-amount/src/localize-monetary-amount.ts

+
+		return groupDigitsAccum(
+			remaining,
+			[ nextGroup.toString().padStart( numDigitsPerGroup, '0' ) ].concat( localDigits ),


String.prototype.padStart will be problematic in terms of support. There are several occurrences.

packages/localize-monetary-amount/test/localize-monetary-amount.js

packages/localize-monetary-amount/src/localize-monetary-amount.ts

jblz · 2019-11-08T19:43:20Z

I just did a cursory scan of this, so please forgive my lack of context.

What's the driver behind putting this in the client? Won't monetary data be coming from the API? What's preventing us from formatting it before we send it to the client (e.g. with our CLDR functionality)?

nbloomf · 2019-11-08T20:04:15Z

Thanks for taking a look @jblz!

What's preventing us from formatting it before we send it to the client?

Nothing, and WPCOM does this already. But this package is an offshoot of composite-checkout, which will be used on other services. That package requires its host client to provide its own formatted currency strings, and this package allows them to do so without changing their own API if needed.

sirreal · 2020-03-10T12:40:24Z

Curious if this is still planned to move forward. There appears to be a lot of valuable work here!

sirbrillig · 2020-03-10T18:31:24Z

I think it would be great if this work could be shared. Maybe we could merge and publish this package, regardless of our current use of it? Or it could be published outside of the calypso monorepo.

sirreal · 2020-03-11T13:37:03Z

With #39786 merged, this will likely require a few build-related changes, but shouldn't be too much work.

Just getting this merged would be a great first step 😁

sirbrillig · 2020-03-11T22:03:05Z

@nbloomf What do you think?

github-actions · 2021-05-05T00:46:04Z

This PR has been marked as stale due to lack of activity within the last 30 days.

sirbrillig · 2021-05-05T17:02:51Z

I still think this would be great to have. We have a bunch of places in calypso where we're forced to get a lot of extra data from the server to avoid client-side math and this would resolve that need.

github-actions · 2021-07-05T00:44:10Z

This PR has been marked as stale due to lack of activity within the last 30 days.

sirbrillig · 2021-07-05T17:06:40Z

I still like it

nbloomf requested a review from sirbrillig October 15, 2019 04:34

nbloomf force-pushed the add/wp-checkout-format-currency branch from a151518 to d88d0b2 Compare October 15, 2019 15:24

nbloomf requested a review from a team as a code owner October 15, 2019 15:24

nbloomf changed the base branch from add/wp-checkout-component to add/wp-checkout-component-test October 15, 2019 15:26

sirbrillig reviewed Oct 17, 2019

View reviewed changes

nbloomf force-pushed the add/wp-checkout-component-test branch from e1ddf84 to 55453d3 Compare October 18, 2019 16:56

nbloomf force-pushed the add/wp-checkout-format-currency branch from fc0b649 to 0d914c7 Compare October 21, 2019 21:05

nbloomf changed the base branch from add/wp-checkout-component-test to add/wp-checkout-component October 21, 2019 21:08

sirbrillig changed the title ~~wp-checkout: Add a function for formatting currency amounts~~ composite-checkout: Add a function for formatting currency amounts Oct 21, 2019

nbloomf changed the base branch from add/wp-checkout-component to master October 22, 2019 17:06

nbloomf changed the base branch from master to add/wp-checkout-component October 22, 2019 17:25

nbloomf force-pushed the add/wp-checkout-format-currency branch from b5b15d2 to 651fefb Compare October 22, 2019 18:13

nbloomf requested a review from a team as a code owner October 22, 2019 18:13

nbloomf changed the base branch from add/wp-checkout-component to master October 22, 2019 18:14

Add a package for localizing monetary amounts

ac5b5a8

This library was written for the in-progress composite-checkout package but has no external dependencies, so we're extracting it as its own package. Features: exact arithmetic and non-breaking spaces

nbloomf force-pushed the add/wp-checkout-format-currency branch from 651fefb to ac5b5a8 Compare November 7, 2019 17:26

nbloomf changed the title ~~composite-checkout: Add a function for formatting currency amounts~~ localize-currency-amount: Utility for locale-aware currency formatting Nov 7, 2019

nbloomf requested a review from sirreal November 7, 2019 17:30

sirreal requested review from a team and removed request for sirreal November 7, 2019 17:37

sirbrillig reviewed Nov 7, 2019

View reviewed changes

sirreal reviewed Nov 7, 2019

View reviewed changes

sirreal assigned nbloomf Nov 7, 2019

sirreal added [Type] Enhancement i18n labels Nov 7, 2019

sirreal added the Packages label Nov 7, 2019

Incorporate feedback

3474e22

sirreal reviewed Nov 8, 2019

View reviewed changes

packages/localize-monetary-amount/src/localize-monetary-amount.ts Outdated Show resolved Hide resolved

nbloomf added 2 commits November 8, 2019 09:47

Incorporate feedback

6f26b3b

Add tsconfig.json and update package.json

b6dcca5

sirbrillig mentioned this pull request Dec 16, 2019

Add payment method api and components to checkout steps woocommerce/woocommerce-blocks#1349

Merged

7 tasks

sirreal mentioned this pull request Mar 10, 2020

Gutenboarding: Update the domains picker to make domain upgrades available #39901

Merged

sarayourfriend changed the base branch from master to trunk November 20, 2020 16:14

matticbot added the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Nov 20, 2020

github-actions bot added the [Status] Stale label May 5, 2021

github-actions bot removed the [Status] Stale label May 5, 2021

github-actions bot added the [Status] Stale label Jul 5, 2021

github-actions bot removed the [Status] Stale label Jul 5, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

localize-currency-amount: Utility for locale-aware currency formatting #36758

localize-currency-amount: Utility for locale-aware currency formatting #36758

nbloomf commented Oct 15, 2019 •

edited

Loading

matticbot commented Oct 15, 2019

matticbot commented Oct 15, 2019 •

edited

Loading

sirbrillig left a comment

sirbrillig commented Oct 18, 2019

sirbrillig commented Oct 21, 2019

nbloomf commented Oct 21, 2019

sirreal left a comment

sirreal Nov 7, 2019

sirreal Nov 7, 2019

jblz commented Nov 8, 2019

nbloomf commented Nov 8, 2019

sirreal commented Mar 10, 2020

sirbrillig commented Mar 10, 2020

sirreal commented Mar 11, 2020

sirbrillig commented Mar 11, 2020

github-actions bot commented May 5, 2021

sirbrillig commented May 5, 2021

github-actions bot commented Jul 5, 2021

sirbrillig commented Jul 5, 2021

localize-currency-amount: Utility for locale-aware currency formatting #36758

Are you sure you want to change the base?

localize-currency-amount: Utility for locale-aware currency formatting #36758

Conversation

nbloomf commented Oct 15, 2019 • edited Loading

Changes proposed in this Pull Request

Testing instructions

matticbot commented Oct 15, 2019

matticbot commented Oct 15, 2019 • edited Loading

sirbrillig left a comment

Choose a reason for hiding this comment

sirbrillig commented Oct 18, 2019

sirbrillig commented Oct 21, 2019

nbloomf commented Oct 21, 2019

sirreal left a comment

Choose a reason for hiding this comment

sirreal Nov 7, 2019

Choose a reason for hiding this comment

sirreal Nov 7, 2019

Choose a reason for hiding this comment

jblz commented Nov 8, 2019

nbloomf commented Nov 8, 2019

sirreal commented Mar 10, 2020

sirbrillig commented Mar 10, 2020

sirreal commented Mar 11, 2020

sirbrillig commented Mar 11, 2020

github-actions bot commented May 5, 2021

sirbrillig commented May 5, 2021

github-actions bot commented Jul 5, 2021

sirbrillig commented Jul 5, 2021

nbloomf commented Oct 15, 2019 •

edited

Loading

matticbot commented Oct 15, 2019 •

edited

Loading