Skip to content

Conversation

@EndPositive
Copy link
Contributor

@EndPositive EndPositive commented Feb 5, 2020

Related Issues

closes #3553

Short Description and Why It's Useful

This pr adds the module currencyformatter.js. You are now able to change the following attributes in config:

  • currencySign: custom currency sign.
  • currencyDecimal: custom char used to display the decimal separator.
  • currencyGroup: custom char used to display the decimal separator.
  • currencyPattern: custom numeric format string. This replaces the priceFormat attribute as you are able to switch sign and value position using this attribute. E.g. #,##0.00! and !#,##0.00. Furthermore, using this pattern, you can specify how many digits the fractional part of the price should have (#,###! for no digits).

When these values are set to null, the locale default will be taken. When one of these are set, the locale will be overwritten. They can be used individually or together.

Which Environment This Relates To

  • Test version (https://test.storefrontcloud.io) - this is a new feature or improvement for Vue Storefront. I've created branch from develop branch and want to merge it back to develop
  • RC version (https://next.storefrontcloud.io) - this is a stabilisation fix for Release Candidate of Vue Storefront. I've created branch from release branch and want to merge it back to release
  • Stable version (https://demo.storefrontcloud.io) - this is an important fix for current stable version. I've created branch from hotfix or master branch and want to merge it back to hotfix

Upgrade Notes and Changelog

  • No upgrade steps required (100% backward compatibility and no breaking changes)
  • I've updated the Upgrade notes and Changelog on how to port existing Vue Storefront sites with this new feature

Contribution and Currently Important Rules Acceptance

Copy link
Collaborator

@pkarw pkarw left a comment

Choose a reason for hiding this comment

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

Thanks that's pretty cool feature

I've checked https://bundlephobia.com/result?p=currencyformatter.js@2.2.0

... and seemingly we can get potentially the same effect with https://www.npmjs.com/package/currency.js

The thing is 1.12 is all about bundle size optimizations so I'm just wondering that maybe you could try out something smaller than currencyformatter.js (?)

@andrzejewsky
Copy link
Contributor

@pkarw I wanted to write the same comment as you did.
@EndPositive it's a cool one, but our regex is totally enough, actually we can improve it somehow(?), but it's not such important that we need to an external library, it's increasing bundle size even though the value is not that big :)

There is Intl.NumberFormat you can move parameters like: minimumFractionDigits and, maximumFractionDigits to the config file and that will be even better and simpler :)


const formatValue = (value, locale) => {
const price = Math.abs(parseFloat(value))
const formatter = new Intl.NumberFormat(locale, { minimumFractionDigits: 2, maximumFractionDigits: 2 })
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 your changes, looks much better but... why not just changing this line? I mean... I think it's totally enough to move just these two options minimumFractionDigits and maximumFractionDigits to the config file and you don't anything else... ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because as said #3553, they also wanted to be able to change the separators.

@andrzejewsky
Copy link
Contributor

@EndPositive you have an error during the build - I'm pretty sure it's because of config file - it's not valid json, please change null's to the default separators

@EndPositive
Copy link
Contributor Author

@andrzejewsky Thanks for the heads up. I changed the config file.

@andrzejewsky andrzejewsky merged commit f0da4a6 into vuestorefront:develop Feb 16, 2020
@andrzejewsky
Copy link
Contributor

@EndPositive thanks!

@dkarlovi dkarlovi mentioned this pull request Jul 7, 2020
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants