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

Honor WooCommerce Settings When Formatting Currency/Price #1342

Merged
merged 11 commits into from Jan 18, 2019

Conversation

3 participants
@jeffstieler
Copy link
Contributor

jeffstieler commented Jan 17, 2019

Fixes #1090.
Fixes #1322.

This PR seeks to format numbers and prices using the store's format settings (WooCommerce > Settings > Currency Options). It makes use of a JS port of PHP's number_format() since the existing JS options are lacking the ability to specify separator characters outside of providing a locale.

In addition to reworking numberFormat() and formatCurrency(), this PR:

  • Adds currency symbol to individual Orders retrieved through the REST API
  • HTML entity decodes currency symbol and price format (to avoid using dangerouslySetInnerHTML)

Screenshots

screen shot 2019-01-17 at 10 15 53 am

Detailed test instructions:

  • Set currency options that don't match the locale of the store (e.g. position="right with space", decimal sep=",", thousand_sep=".", precision="3" for USD)
  • Verify that all numbers and prices use this format in reports, including the Y-axis of charts
@timmyc
Copy link
Collaborator

timmyc left a comment

This is great to see! We already had some feedback around currency formatting on one of the release posts, so this will save quite a bit of bug reports over the coming months - thanks Jeff!

Had a few questions in the code, but overall looking fantastic. Could you also add a note to CHANGELOG.md in the components and currency packages folder? Since we just released them yesterday, you'll probably have to start new minor version sections like `1.4.1 ( unreleased ) in https://github.com/woocommerce/wc-admin/blob/master/packages/components/CHANGELOG.md

if ( isNaN( number ) ) {
return '';
}
return new Intl.NumberFormat( locale ).format( number );

const decimalSeparator = get( wcSettings, [ 'currency', 'decimal_separator' ], '.' );

This comment has been minimized.

@timmyc

timmyc Jan 17, 2019

Collaborator

Nice, I like the fallbacks here in the event of the wcSettings global not being around.

const decimalSeparator = get( wcSettings, [ 'currency', 'decimal_separator' ], '.' );
const thousandSeparator = get( wcSettings, [ 'currency', 'thousand_separator' ], ',' );

if ( null === precision ) {

This comment has been minimized.

@timmyc

timmyc Jan 17, 2019

Collaborator

Should we also add an || clause here to validate that precision if defined, is a number - otherwise use this same logic?

This comment has been minimized.

@jeffstieler

jeffstieler Jan 17, 2019

Author Contributor

Update in d2cb3f7.

@@ -16,4 +16,16 @@ describe( 'numberFormat', () => {
it( 'should accept a string', () => {
expect( numberFormat( '10000' ) ).toBe( '10,000' );
} );

it( 'maintains all decimals if no precision specified', () => {

This comment has been minimized.

@timmyc

timmyc Jan 17, 2019

Collaborator

Good test additions, thanks. Also related to the comment about parsing/validating precision to a number - might be another good test case to have here.

This comment has been minimized.

@jeffstieler

jeffstieler Jan 17, 2019

Author Contributor

Added in 48249ea.

@@ -757,6 +760,22 @@ public static function install() {
add_action( 'woocommerce_after_register_post_type', array( __CLASS__, 'regenerate_report_data' ), 20 );
}
/**

This comment has been minimized.

@timmyc

timmyc Jan 17, 2019

Collaborator

Nice solution here. Since we are versioning the API to v4 with wc-admin, what do you think about just including this in the core endpoint logic? Obviously out of scope of the PR but curious to hear your thoughts.

if ( 'number' !== typeof number ) {
number = parseFloat( number );
export function formatCurrency( number, currencySymbol ) {
// default to wcSettings if currency symbol is not passed in

This comment has been minimized.

@timmyc

timmyc Jan 17, 2019

Collaborator

Maybe this comment should also say if wcSettings is not defined, $ is the fallback to the fallback 🕳

This comment has been minimized.

@jeffstieler

jeffstieler Jan 17, 2019

Author Contributor

Done in e32deda.

@Aljullu
Copy link
Contributor

Aljullu left a comment

Closing two issues in one PR. 😍

This is testing well for me, couldn't find any number that is not correctly formatted. I just added a couple of comments inline with the code.

return formattedNumber + decimalSeparator + decimals;
}

return formattedNumber;

This comment has been minimized.

@Aljullu

Aljullu Jan 17, 2019

Contributor

Actually, this block of code can be simplified, if I'm not wrong.

Once you have decimals, you can infer the precision from there:

	if ( null === precision ) {
		const decimals = get( number.toString().split( '.' ), [ 1 ], '' );
		precision = decimals.length;
	}

I didn't do a lot of testing, but I think it should work and tests pass.

This comment has been minimized.

@jeffstieler

jeffstieler Jan 17, 2019

Author Contributor

Nice! Updated in d2cb3f7.

* @see https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/NumberFormat
* @param {Number|String} number number to format
* @returns {?String} A formatted string.
* @see https://locutus.io/php/strings/number_format/

This comment has been minimized.

@Aljullu

Aljullu Jan 17, 2019

Contributor

I get this screen when I visit this page (both in Firefox and Chrome):
image

HTTP version seems to work fine.

This comment has been minimized.

@jeffstieler

jeffstieler Jan 17, 2019

Author Contributor

I'll swap to the HTTP version.

@jeffstieler
Copy link
Contributor Author

jeffstieler left a comment

Could you also add a note to CHANGELOG.md in the components and currency packages folder?

Done in 8f0d27c. This ready for another review.

return formattedNumber + decimalSeparator + decimals;
}

return formattedNumber;

This comment has been minimized.

@jeffstieler

jeffstieler Jan 17, 2019

Author Contributor

Nice! Updated in d2cb3f7.

const decimalSeparator = get( wcSettings, [ 'currency', 'decimal_separator' ], '.' );
const thousandSeparator = get( wcSettings, [ 'currency', 'thousand_separator' ], ',' );

if ( null === precision ) {

This comment has been minimized.

@jeffstieler

jeffstieler Jan 17, 2019

Author Contributor

Update in d2cb3f7.

@@ -16,4 +16,16 @@ describe( 'numberFormat', () => {
it( 'should accept a string', () => {
expect( numberFormat( '10000' ) ).toBe( '10,000' );
} );

it( 'maintains all decimals if no precision specified', () => {

This comment has been minimized.

@jeffstieler

jeffstieler Jan 17, 2019

Author Contributor

Added in 48249ea.

if ( 'number' !== typeof number ) {
number = parseFloat( number );
export function formatCurrency( number, currencySymbol ) {
// default to wcSettings if currency symbol is not passed in

This comment has been minimized.

@jeffstieler

jeffstieler Jan 17, 2019

Author Contributor

Done in e32deda.

@timmyc

timmyc approved these changes Jan 18, 2019

Copy link
Collaborator

timmyc left a comment

Thanks for the updates, great refactor suggestion @Aljullu - this is still testing great for me.

@jeffstieler jeffstieler merged commit 3c370dc into master Jan 18, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@jeffstieler jeffstieler added this to 🥋Sprint 11 Done in Isotope via automation Jan 18, 2019

@jeffstieler jeffstieler deleted the fix/1090-currency-settings-formatting branch Jan 18, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment