Implement @woocommerce/price-format dependency #29

nielslange · 2021-10-22T09:11:14Z

Fixes #28

Note

The remaining price should be formatted using the @woocommerce/price-format dependency, which is included in the WooCommerce Blocks plugin.

Testing instructions

Before all test cases

Check out this PR.
Create a test page and add the Cart block.
Insert the Free Shipping Progress Bar block into the Cart block and save the page.
Set the free shipping value within the Free Shipping Progress Bar block to 1200.
Head over to the frontend and add products to your cart.
Ensure the cart total is higher than 1000 (to test the thousand separator) and below 1200 (to see the remaining price).

Test 0 decimals currency

Go to WP Admin » WooCommerce » Settings » Currency options and use the following settings:
- Currency: Indonesian rupiah (Rp)
- Currency position: Left
- Thousand separator: .
- Decimal separator: ,
- Number of decimals: 0
Go to the cart and verify that the price is formatted as expected.

Test 2 decimals currency

Go to WP Admin » WooCommerce » Settings » Currency options and use the following settings:
- Currency: USD $
- Currency position: Left + space
- Thousand separator: ,
- Decimal separator: .
- Number of decimals: 2
Go to the cart and verify that the price is formatted as expected.

Test 3 decimals currency

Go to WP Admin » WooCommerce » Settings » Currency options and use the following settings:
- Currency: Libyan Dinar
- Currency position: Right + space
- Thousand separator: .
- Decimal separator: ,
- Number of decimals: 3
Go to the cart and verify that the price is formatted as expected.

senadir

I'm finding it hard to follow what's happening, I left some comments about (what I believe to be) redundant code and eager abstraction.

To my mind, this is how it should be done:

Convert the string provided from "50" to 5000. This is, to my knowledge, not something that `@woocommerce/format-price provide, but is something we're doing in Filter By Product block.
Do the math.
Format result back.

This should all happen directly on the component, no need for extra utils that are one time use, depending on how or easy 1 is, it might get its own utility function to reduce clutter, the rest can stay.

senadir · 2021-12-15T10:42:05Z

src/components/progress-label/index.js

+	const currentTotal = getCurrentTotal( cart );
+	const minorUnit = getMinorUnit( cart );
+	const remaining = getRemaining( freeShippingFrom, currentTotal, minorUnit );
+	const currency = getCurrencyFormat( cart );


This seems like an early abstraction that shouldn't happen yet until you actually need it, further more, some of functions are useless if you check for cart here.

I'm not sure what you mean with the early abstraction. Could you elaborate on that? And could you tell me which functions you think are useless here? I created them to extract the cart total, the minor units and the currency format from the cart object, if a cart object is available.

src/util.js

senadir · 2021-12-15T14:48:56Z

src/util.js

+ * @param {Object} cart The cart object.
+ * @returns {Object} The currency format object.
+ */
+export function getCurrencyFormat( cart ) {


There seems to be a lot of redundant logic here, you can get currency by using getCurrencyFromPriceResponse( cart.cartTotals )

Good catch. I removed my function getCurrencyFormat() and I'm using getCurrencyFromPriceResponse() instead.

nielslange · 2021-12-16T05:03:09Z

I'm finding it hard to follow what's happening, I left some comments about (what I believe to be) redundant code and eager abstraction.

To my mind, this is how it should be done:

Convert the string provided from "50" to 5000. This is, to my knowledge, not something that `@woocommerce/format-price provide, but is something we're doing in Filter By Product block.

Do the math.

Format result back.

That's pretty much what I'm doing here. As for converting "50" to 5000, this would only be correct when the merchant selected 2 decimals to display. For 3 decimals, the minor unit price should be 50000 and for 0 decimals, the minor unit price should remain 50.

This should all happen directly on the component, no need for extra utils that are one time use, depending on how or easy 1 is, it might get its own utility function to reduce clutter, the rest can stay.

I moved the helper functions into util.js, as I'm calling the function getCurrentTotal() both in src/components/progress-bar/index.js and in src/components/progress-label/index.js. In addition, it keeps the individual components leaner which, in my opinion, improves both readability and maintainability.

As I'm using getMinorUnit() and getRemaining() only in src/components/progress-label/index.js, I could technically move these two functions directly into the corresponding component. I decided against that, as I believe it makes more sense to have all related functions in one file instead of spreading them across the project.

I'm looking forward to hearing your thoughts on my feedback regarding your review and the additional notes I added before.

…dependency

Update dependency @wordpress/components to v19.1.5

mikejolley

Changes look good, and Nadirs feedback was resolved. I made some small suggestions but nothing blocking. Test coverage also good.

src/util.js

Update wordpress monorepo

Remove obsolete icon

…of github.com:woocommerce/woocommerce-free-shipping-progress-bar-block into add/28-implement-@woocommerce-price-format-dependency # Conflicts: # src/components/progress-bar/index.js # src/components/progress-label/index.js

nielslange · 2022-01-28T14:08:56Z

I applied all changes and fixed the integration tests, which broke due to adding @woocommerce/price-format in this change. Do you want to run another review, @mikejolley?

nielslange requested a review from senadir October 22, 2021 09:14

Base automatically changed from add/25-register-block-in-cart-i2 to trunk October 25, 2021 10:00

nielslange marked this pull request as draft November 4, 2021 11:19

nielslange changed the title ~~Add helper function to format price~~ Implement @woocommerce/price-format dependency Nov 15, 2021

nielslange marked this pull request as ready for review November 26, 2021 14:49

Implement @woocommerce/price-format dependency

6fc3130

nielslange force-pushed the add/28-implement-@woocommerce-price-format-dependency branch from af79156 to 6fc3130 Compare November 27, 2021 07:57

Update dependency @wordpress/components to v19.1.5

b6204e9

senadir requested changes Dec 15, 2021

View reviewed changes

nielslange added 3 commits December 16, 2021 11:33

Remove obsolete checks

e0ab51f

Add default value showing insufficient cart totals progress bar

8d3ba4c

Replace getCurrencyFormat() with getCurrencyFromPriceResponse()

e85f38c

nielslange and others added 4 commits December 20, 2021 13:59

Merge branch 'trunk' into add/28-implement-@woocommerce-price-format-…

fce48d6

…dependency

Merge branch 'trunk' into renovate/wordpress-monorepo

aadbd61

Merge pull request #36 from woocommerce/renovate/wordpress-monorepo

f7518da

Update dependency @wordpress/components to v19.1.5

Update wordpress monorepo

2ce76b9

mikejolley approved these changes Jan 24, 2022

View reviewed changes

src/util.js Outdated Show resolved Hide resolved

src/util.js Outdated Show resolved Hide resolved

src/util.js Outdated Show resolved Hide resolved

nielslange added 12 commits January 24, 2022 19:06

Merge pull request #38 from woocommerce/renovate/wordpress-monorepo

9974eaf

Update wordpress monorepo

Remove obsolete icon

63aac05

Merge pull request #46 from woocommerce/fix/45-remove-obsolete-icon

c48fcc2

Remove obsolete icon

Adjust getMinorUnit() function

bff7992

Use default current totals value from block.json

e1c27dc

Remove obsolete attributes from tests

d272664

Implement @woocommerce/price-format dependency

fcefbeb

Remove obsolete checks

30f5a83

Add default value showing insufficient cart totals progress bar

5c7f6ab

Replace getCurrencyFormat() with getCurrencyFromPriceResponse()

2cf0023

Adjust getMinorUnit() function

1d9e48d

Use default current totals value from block.json

706a248

nielslange added 4 commits January 25, 2022 17:06

Remove obsolete attributes from tests

96d7c07

Interim Jest mock commit

0aa53b1

Fix failing integration test

995f5a0

nielslange mentioned this pull request Feb 3, 2022

Make color editable #14

Merged

nielslange requested a review from opr March 30, 2022 16:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement @woocommerce/price-format dependency #29

Implement @woocommerce/price-format dependency #29

nielslange commented Oct 22, 2021 •

edited

Loading

senadir left a comment

senadir Dec 15, 2021

nielslange Dec 16, 2021

senadir Dec 15, 2021

nielslange Dec 16, 2021

nielslange commented Dec 16, 2021

mikejolley left a comment

nielslange commented Jan 28, 2022

Implement @woocommerce/price-format dependency #29

Are you sure you want to change the base?

Implement @woocommerce/price-format dependency #29

Conversation

nielslange commented Oct 22, 2021 • edited Loading

Note

Testing instructions

Before all test cases

Test 0 decimals currency

Test 2 decimals currency

Test 3 decimals currency

senadir left a comment

Choose a reason for hiding this comment

senadir Dec 15, 2021

Choose a reason for hiding this comment

nielslange Dec 16, 2021

Choose a reason for hiding this comment

senadir Dec 15, 2021

Choose a reason for hiding this comment

nielslange Dec 16, 2021

Choose a reason for hiding this comment

nielslange commented Dec 16, 2021

mikejolley left a comment

Choose a reason for hiding this comment

nielslange commented Jan 28, 2022

nielslange commented Oct 22, 2021 •

edited

Loading