-
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
Plans: add support for client-side currency formatting #6046
Conversation
f49bbc8
to
d3c2ce6
Compare
} ); | ||
it( 'EUR', () => { | ||
const money = currencyFormatter( 9800900.32, { code: 'EUR' } ); | ||
expect( money ).to.equal( '9 800 900,32 €' ); |
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.
This format doesn't look right. It's missing some .
s
9.800.900,32€
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.
This depends on the country, some will use spaces for the thousand separator. We can still update the default here. https://docs.oracle.com/cd/E19455-01/806-0169/overview-9/index.html
* @returns {String} A formatted string. | ||
*/ | ||
export default function currencyFormatter( number, options = {} ) { | ||
const defaults = findCurrency( options.code ); |
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.
options.code
should be documented?
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.
Updated in b7b2d14
d591c9d
to
b7b2d14
Compare
Unfortunately, interpolated components is not available as a main dependency. I'll look into writing our own currency formatter that gives us more detailed information. |
*/ | ||
export default function currencyFormatter( number, options = {} ) { | ||
const { symbol, thousandsSeparator } = findCurrency( options.code ); | ||
return formatter.format( number, Object.assign( { symbol, thousand: thousandsSeparator }, options ) ); |
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.
Please don't feel like I'm trying to be critical - this is just a style note related to L41 below, but you can avoid the Object.assign()
here too. This is just my personal preference and nothing more 😄
return formatter.format( number, { symbol, thousand: thousandsSeparator, ...options } );
If you put ...options
first, then if it has symbol
or thousand
properties they will be overwritten.
/me ❤️ the spread operator
04faa2d
to
6150555
Compare
6150555
to
96155c8
Compare
I've also renamed the util lib from currency-formatter to format-currency, since I don't actually return a formatter object. |
@@ -43,7 +48,7 @@ export const getPlan = createSelector( | |||
*/ | |||
export function getPlanRawPrice( state, productId, isMonthly = false ) { | |||
const plan = getPlan( state, productId ); | |||
if ( ! plan || ! ( plan.raw_price || plan.raw_price === 0 ) ) { | |||
if ( get( plan, 'raw_price', -1 ) < 0 ) { |
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.
To be honest, this looks rather strange and complicated. I liked your previous solution better. It was more readable. But that's just my opinion and this is a minor detail. :)
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.
Yeah I think an existential operator would read better, but it basically boils down to fetch plan.raw_price if possible, otherwise default to -1.
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.
I see. My point is that a programmer who didn't read the lodash docs or is not super familiar with get
won't be able to parse this line quickly. Or to phrase it differently: I did not understand it at first sight, only after going through the lodash docs. :)
Let's 🚢 this. |
This PR adds a
currency-formatter librarycurrency formatting library which gives us the ability to display money properly on the client. This is to accommodate for cases where we have raw pricing data, but the server formatted price is too limiting for our use case. This comes up in monthly pricing, or when we wish to wrap markup around things like the currency symbol for a prettier display.~~As to why I've pulled in another dependency for this:
If we think the dependency is too large, I'd be open to writing a similar one from scratch.~~
I've also updated the plans-features component to use the currency formatter as an advanced example.

Testing Instructions
cc @lamosty @dmsnell @rralian @retrofox
Test live: https://calypso.live/?branch=try/currency-formatter