Load size units to show them as a suffix of shipping dimensions fields #34856

mdperez86 · 2022-09-27T19:38:40Z

All Submissions:

Have you followed the WooCommerce Contributing guideline?
Does your code follow the WordPress' coding standards?
Have you checked to ensure there aren't other open Pull Requests for the same update/change?

Changes proposed in this Pull Request:

Load dimensions and weight units to show them as a suffix of shipping dimensions fields, so the user will know a reference of which unit of the product dimensions will be used. These units are comming from WooCommerce > Settings > Products configuration.

Partialy closes #34329.

How to test the changes in this Pull Request:

When in the new Product page
A Shipping Dimensions section should be shown with the dimensions and weight units loaded as suffixes of each input field according the WooCommerce > Settings > Products configuration.

Other information:

Have you added an explanation of what your changes do and why you'd like us to include them?
Have you written new tests for your changes, as applicable?
Have you successfully run tests with your changes locally?
Have you created a changelog file for each project being changed, ie pnpm changelog add --filter=<project>?

FOR PR REVIEWER ONLY:

I have reviewed that everything is sanitized/escaped appropriately for any SQL or XSS injection possibilities. I made sure Linting is not ignored or disabled.

github-actions · 2022-09-27T20:48:12Z

Test Results Summary

Commit SHA: fa2e2cb

Test 🧪	Passed ✅	Failed 🚨	Broken 🚧	Skipped ⏭️	Unknown ❔	Total 📊	Duration ⏱️
API Tests	118	0	0	2	0	120	0m 56s
E2E Tests	186	1	0	2	0	189	13m 18s

To view the full API test report, click here.
To view the full E2E test report, click here.
To view all test reports, visit the WooCommerce Test Reports Dashboard.

louwie17

Nice work @mdperez86, this tested well and looked good!
I did leave several comments, one small one around styling and a couple in relation to the BaseControl wrapper, which I don't think is necessary.

Outside of that, nice work on the Form updates and consolidating some of the styling and removing some of the duplicate styling.

louwie17 · 2022-09-28T19:12:58Z

packages/js/components/src/form/form.tsx

 	const resetForm = (
-		newInitialValues: Values,
-		newChangedFields = {},
+		newInitialValues = {} as Values,


Removing the newChangedFields param is probably ok, given I added the additional params just a month ago.
We do have to be careful removing those in the future, for backwards compability.

louwie17 · 2022-09-28T19:39:13Z

plugins/woocommerce-admin/client/products/layout/product-section-layout.scss

 	}

 	&__content {
-		padding: $gap-large;


It's fine to remove some of these and replace it with the Card component, but the Card component styling is slightly different. It's top/bottom margin is smaller 16px versus 24px and the border-radius a little bit less.

So maybe add the padding and border-radius back for this and target the .components-card specifically.

louwie17 · 2022-09-28T19:47:41Z

plugins/woocommerce-admin/client/products/sections/pricing-section.tsx

+					<BaseControl
+						{ ...regularPriceProps }
+						id="product_pricing_regular_price"
+					>


Is there a reason why these fields need to be wrapped in a BaseControl component? and why the BaseControl component needs the same props?

Left another comment further down in relation to this, I don't think it's necessary.

now it's a little more explicit why I was using BaseControl

louwie17 · 2022-09-28T19:56:51Z

plugins/woocommerce-admin/client/products/sections/product-shipping-section.tsx

-						label={ __( 'Shipping class', 'woocommerce' ) }
-						{ ...getTextControlProps(
-							getInputProps( 'shipping_class' )
+		<>


I don't think this Fragment is necessary is it? Given everything is wrapped within the ProductSectionLayout.

Ohh, that's right. Thanks.

louwie17 · 2022-09-28T20:08:11Z

plugins/woocommerce-admin/client/products/sections/product-shipping-section.tsx

+								</p>
+								<div className="product-shipping-section__container">
+									<div>
+										<BaseControl


I don't think the BaseControl is needed for wrapping each InputControl. The logic within BaseControl is already integrated within the InputControl if you look at the label code for example:
BaseControl: https://github.com/WordPress/gutenberg/blob/trunk/packages/components/src/base-control/index.tsx#L52-L73
InputControl: https://github.com/WordPress/gutenberg/blob/trunk/packages/components/src/input-control/label.tsx

Was this recommended somewhere else?

I wrapped each InputControl with BaseControl for 2 reasons.

Other TextControls we are using they are all wrapped within the BaseControl as you can see here.

InputControl does not have the help feature which is used to show error messages and it's spaces styles are not the same as the TextControl we are using.

Other TextControls we are using they are all wrapped within the BaseControl as you can see here.

Yeah, although the InputControl is suppose to replace this TextControl so maybe the help feature needed as part of your second point will still get added to the InputControl. I guess we should keep an eye out during any WordPress updates for this.

I think it would be worth to make our own InputControl wrapper that acts much like the TextControl you referenced, that wraps the BaseControl already. So in the case were WordPress does add this on update we only have one spot to update, and not a bunch of places.
But I acknowledge this would be out of the scope of this PR and can be done as a follow up.

louwie17 · 2022-09-28T20:18:46Z

packages/js/components/src/form/form.tsx

-			const valuesToSet: Values = {} as Values;
-			valuesToSet[ name as keyof Values ] = value;
-			setValues( valuesToSet );
+			setValues( _setWith( { ...values }, name, value, _clone ) );


cc @joshuatf in relation to the lodash usage, I know you are less of a fan of using lodash.
This does allow us to reference fields in this manner dimensions.width which is very handy, for the shipping field for example.
We could potentially implement this manually, but given lodash would be much more stable, I would be fine with using this (given we aren't pruning it out yet).
But let me know if you have any strong sense against this?

louwie17

Looks good @mdperez86, I just left one comment with a small bug I found now with separating the props out for the shipping dimensions. The onBlur is missing, which is required for the help text to show.

louwie17 · 2022-09-29T15:45:50Z

plugins/woocommerce-admin/client/products/sections/product-shipping-section.tsx

+													'woocommerce'
+												)
+											) }
+											onChange={ ( value: string ) =>


You also want to pass in the onBlur callback to the InputControl as otherwise the help text doesn't show up.

louwie17

Nice work @mdperez86, it's looking great now, thanks for your hard work on this 👍

Change the FormComponent to support input name with dot notation like dimensions.width Add the dimension controls to the product form

…Body instead

…alues

…ontrol

louwie17

Re-approving after rebase.

github-actions · 2022-09-29T17:54:18Z

Hi @mdperez86, thanks for merging this pull request. Please take a look at these follow-up tasks you may need to perform:

Add the release: add testing instructions label

mdperez86 added focus: product management MVP labels Sep 27, 2022

mdperez86 requested a review from a team September 27, 2022 19:38

mdperez86 force-pushed the add/34329-load-size-units branch from e2bee57 to 50d217e Compare September 28, 2022 18:55

louwie17 reviewed Sep 28, 2022

View reviewed changes

mdperez86 requested a review from louwie17 September 29, 2022 15:30

louwie17 reviewed Sep 29, 2022

View reviewed changes

mdperez86 force-pushed the add/34329-load-size-units branch from b525da8 to 458fcb3 Compare September 29, 2022 16:12

louwie17 previously approved these changes Sep 29, 2022

View reviewed changes

mdperez86 added 9 commits September 29, 2022 14:12

Add dimensions to shipping section

1da3d17

Change the FormComponent to support input name with dot notation like dimensions.width Add the dimension controls to the product form

Remove custom styles from FormSection component and use Card and Card…

372d662

…Body instead

Add dimension fields formating using the woo number global settings

984174e

Add dimension fields validations

361b2ab

Solve conflics from rebase

d2000ea

Add changelogs

1ec4f01

Fix resetForm function to consider the passed params and the initialV…

86bb55f

…alues

Load size units to show it as a suffix of shipping dimensions fields

389ead4

Fix card styles according the disign and pass props manually to BaseC…

fa2e2cb

…ontrol

mdperez86 dismissed louwie17’s stale review via fa2e2cb September 29, 2022 17:13

mdperez86 force-pushed the add/34329-load-size-units branch from 458fcb3 to fa2e2cb Compare September 29, 2022 17:13

louwie17 approved these changes Sep 29, 2022

View reviewed changes

mdperez86 merged commit 7fe05d6 into trunk Sep 29, 2022

mdperez86 deleted the add/34329-load-size-units branch September 29, 2022 17:54

github-actions bot added this to the 7.1.0 milestone Sep 29, 2022

mdperez86 mentioned this pull request Sep 29, 2022

Add dimensions to shipping section #34821

Closed

8 tasks

github-actions bot mentioned this pull request Nov 10, 2022

Test WooCommerce 7.1 UVLabs/location-picker-at-checkout-for-woocommerce#55

Closed

Load size units to show them as a suffix of shipping dimensions fields #34856

Load size units to show them as a suffix of shipping dimensions fields #34856

Uh oh!

Conversation

mdperez86 commented Sep 27, 2022

All Submissions:

Changes proposed in this Pull Request:

How to test the changes in this Pull Request:

Other information:

FOR PR REVIEWER ONLY:

Uh oh!

github-actions bot commented Sep 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test Results Summary

Uh oh!

louwie17 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

louwie17 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mdperez86 Sep 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

louwie17 left a comment

Choose a reason for hiding this comment

Uh oh!

louwie17 left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Sep 29, 2022 • edited by mdperez86 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

github-actions bot commented Sep 27, 2022 •

edited

Loading

mdperez86 Sep 29, 2022 •

edited

Loading

github-actions bot commented Sep 29, 2022 •

edited by mdperez86

Loading