Show decimals in mini cart regular price would not have decimals #5125
Conversation
nielslange
added
status: needs review
type: bug
rubikuserbot
requested review from
a team and
opr
and removed request for
a team
|
Size Change: +153 B (0%) Total Size: 1.12 MB
ℹ️ View Unchanged
|
nielslange
changed the title
There was a problem hiding this comment.
Works well for me, cool use of regex and I love the tests you added 👍🏼
Besides what Tung has already mentioned, I've added a couple of comments relating to the documentation of functions, but these are only minor edits so I'll approve.
Please reconsider the testing instructions, I don't think there's any value getting people to experience the bug in trunk first before testing. Especially not for user-facing testing.
github-actions
bot
added
status: ready to merge
and removed
status: needs review
labels
|
@nielslange I only just noticed this PR and wanted to note that these issues were very similar: I fixed the formatting in #5188 This is going to impact this PR. I also needed to separate the before/after decimal part and implemented a |
Thanks for letting me know, @mikejolley. I just checked #5188 and noticed that it would break this PR, which is pretty much done. As this PR fixes both #5018 and #5019, I reverted your changes for now.
In p1636638621033700/1636637781.030600-slack-C8X6Q7XQU, @senadir mentioned that |
I'll adjust the testing instructions accordingly, @opr. @dinhtungdu & @opr Do you two want to do a last review of this PR, or shall I go ahead and merge it? |
There was a problem hiding this comment.
From my point of view it works and the code looks fine, so I can approve! I would like to cc @mikejolley since this affects work you did (Niels has reverted one of your PRs) please can you check over it too?
I did not use I think you've also reverted a fix where I added: I found that on the first load, |
| ${ '' } | ${ '' } | ||
| ${ null } | ${ '' } | ||
| ${ undefined } | ${ '' } | ||
| `( | ||
| 'correctly formats price given "$value" only as "$expected"', |
There was a problem hiding this comment.
Having $expected was useful when viewing the output of the tests.
| @@ -76,7 +76,7 @@ export const getCurrencyFromPriceResponse = ( | |||
| | Record< string, never > | |||
| | CartShippingPackageShippingRate | |||
| ): Currency => { | |||
| if ( ! currencyData?.currency_code ) { | |||
There was a problem hiding this comment.
This prevents invalid (default state) currencyData taking precedence over the default siteCurrencySettings.
| * @param {number} minorUnit The number of decimals to display. | ||
| * @return {string} The extracted decimal value. | ||
| */ | ||
| export const getDecimalValue = ( |
There was a problem hiding this comment.
These functions seem to be parsing the decimal value, and formatting it, is that correct? The splitDecimal I added seemed more easy to understand. Can we not do the formatting part (adding 0's etc) later using a separate function? cc @senadir
|
Closing this PR in benefit of #5219. |
nielslange
deleted the
fix/5019-show-decimals-in-mini-cart-for-0-price
branch
Fixes #5018 & #5019
Problem
In #5019, @dinhtungdu reported that the decimals in the mini cart block are not visible, when the price is 0. After viewing the unit tests of this fix, I noticed that this problem not only affects a $0 price, but also a price that has no decimals, such as $15.00. This price will then appear as $15 in the mini cart.
Reason
To format the price, we convert the regular price into a decimal price by multiplying the regular price with 10 to the power of the number of decimals. Let's say the merchant sets the number of decimals to 2 and the cart contains products with a value of $23.05. We'd then convert the regular price to a decimal price as follows:
Now, let's take a look what happens when we convert a $0 price:
To keep the decimal zeros, we'd expect the decimal price
000, but we get the decimal price0.Let's also take a quick look what happens when we convert a $15 price:
So far, this is ok. However, when formatting the price in https://github.com/woocommerce/woocommerce-gutenberg-products-block/blob/6cb995c49f11ee59f03951a57ab8a93b6de01753/packages/prices/utils/price.ts#L137, the decimals get lost:
Solution
To solve this problem, I'm splitting the price into an integer and a decimal part. That way, I can ensure that the decimals do not get lost independent if the cart holds a $0 price or a higher price, but without decimals, e.g. $15.
Testing
Automated testing
Manual testing
Test $0.00 value
WP Admin » WooCommerce » Setting » General » Currency options.2.$0.00.$0.00.Test number of decimals
WP Admin » WooCommerce » Setting » General » Currency options.3.$0.000.$0.000.Test separators
WP Admin » WooCommerce » Setting » General » Currency options..and the decimal separator to,..and the decimal separator as,.Test currency and currency position
WP Admin » WooCommerce » Setting » General » Currency options.Euro (€)and the currency position toRight with space.Euro (€)and that it's place on the right-hand side, including a space.Changelog