Skip to content
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

Merged
merged 18 commits into from
Jun 17, 2016

Conversation

gwwar
Copy link
Contributor

@gwwar gwwar commented Jun 14, 2016

This PR adds a currency-formatter library currency 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:

  1. number.toLocaleString is unsupported in Safari, and has inconsistent browser results
  2. currency display has a number of edge cases and at least warrants its own lib module.
    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.
screen shot 2016-06-15 at 3 28 19 pm

screen shot 2016-06-14 at 3 36 09 pm

screen shot 2016-06-14 at 3 35 51 pm

Testing Instructions

cc @lamosty @dmsnell @rralian @retrofox

Test live: https://calypso.live/?branch=try/currency-formatter

@gwwar gwwar added [Feature] Plans & Upgrades All of the plans on WordPress.com and flow for upgrading plans. [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Jun 14, 2016
@gwwar gwwar added this to the Plans: Maintenance milestone Jun 14, 2016
@gwwar gwwar self-assigned this Jun 14, 2016
@gwwar gwwar force-pushed the try/currency-formatter branch 4 times, most recently from f49bbc8 to d3c2ce6 Compare June 14, 2016 23:56
} );
it( 'EUR', () => {
const money = currencyFormatter( 9800900.32, { code: 'EUR' } );
expect( money ).to.equal( '9 800 900,32 €' );
Copy link
Member

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€

Copy link
Contributor Author

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

@dmsnell dmsnell added [Status] Needs Author Reply and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Jun 15, 2016
* @returns {String} A formatted string.
*/
export default function currencyFormatter( number, options = {} ) {
const defaults = findCurrency( options.code );
Copy link
Contributor

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated in b7b2d14

@gwwar
Copy link
Contributor Author

gwwar commented Jun 15, 2016

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 ) );
Copy link
Member

@dmsnell dmsnell Jun 15, 2016

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

@gwwar gwwar added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. and removed [Status] In Progress labels Jun 15, 2016
@gwwar gwwar force-pushed the try/currency-formatter branch from 04faa2d to 6150555 Compare June 17, 2016 00:00
@gwwar gwwar force-pushed the try/currency-formatter branch from 6150555 to 96155c8 Compare June 17, 2016 00:03
@gwwar
Copy link
Contributor Author

gwwar commented Jun 17, 2016

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 ) {
Copy link
Contributor

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. :)

Copy link
Contributor Author

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.

https://lodash.com/docs#get

Copy link
Contributor

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. :)

@lamosty
Copy link
Contributor

lamosty commented Jun 17, 2016

Let's 🚢 this.

@lamosty lamosty added [Status] Ready to Merge and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Jun 17, 2016
@gwwar
Copy link
Contributor Author

gwwar commented Jun 17, 2016

Thanks for the reviews @lamosty @dmsnell @aduth !

@gwwar gwwar merged commit 6f56c8f into master Jun 17, 2016
@gwwar gwwar deleted the try/currency-formatter branch June 17, 2016 18:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Plans & Upgrades All of the plans on WordPress.com and flow for upgrading plans.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants