Adapt the width of the pricing fields #35545

nathanss · 2022-11-10T19:15:22Z

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:

Changes the width of the pricing fields based on the screen's width.

I'm not sure how we want to determine the width of the fields. @jarekmorawski mentioned the 1032px width rule on issue #35180, so that's what I adopted in the first place.

Closes #35538.

This PR is a very minor change/addition and does not require testing instructions (if checked you can ignore/remove the next section).

How to test the changes in this Pull Request:

Go to the new Add Product page (/wp-admin/admin.php?page=wc-admin&path=%2Fadd-product)
Check if the width of the pricing fields are 50% when the screen width is larger than 1032px
Adjust the screen width to a smaller value and check if the width of the pricing fields turn into 100%

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 created a changelog file for each project being changed, ie pnpm --filter=<project> changelog add?

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-11-10T19:40:45Z

Test Results Summary

Commit SHA: db05dc2

Test 🧪	Passed ✅	Failed 🚨	Broken 🚧	Skipped ⏭️	Unknown ❔	Total 📊	Duration ⏱️
API Tests	260	0	0	2	0	262	1m 16s
E2E Tests	186	0	0	6	0	192	15m 20s

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.

mattsherman

Nice start on this @nathanss! I left a few comments/pointers to things you should consider to make this implementation a bit more robust. Mainly from a naming standpoint and using our standard breakpoint mixin and a standard breakpoint value.

mattsherman · 2022-11-12T01:24:50Z

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

 	}
 }
+
+.woocommerce-field-width {


This class name feels too generic (I recognize we have other generic class names in this file... we should clean those up too in a future PR). Since it is in the pricing section, we should name it something like:

product-pricing-section__price-field

Or, move it somewhere more general, like https://github.com/woocommerce/woocommerce/blob/736593ef1526ac1aa5495851789690b26eb2d657/plugins/woocommerce-admin/client/products/product-page.scss and call it something like:

.woocommerce-add-product, .woocommerce-edit-product { .half-width-field { // props go here } }

I'm not sold on the half-width-field... part of me likes it and part of me thinks we should list our the classname of every specific type of field that is going to use this instead. What do you think?

I'd say we keep like half-width-field specially since it seems we will be adding this to more fields in the future. We can change in the future if needed

mattsherman · 2022-11-12T01:33:10Z

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

 }
+
+.woocommerce-field-width {
+	@media only screen and (max-width: 1032px) {


We should use the following instead:

@include breakpoint( '<960px' ) { width: 100%; }

Using the breakpoint mixin from https://github.com/woocommerce/woocommerce/blob/112b9ac67a12a86e33d1d9d47027c76f17522083/packages/js/internal-style-build/abstracts/_breakpoints.scss

Since that mixin doesn't support the 1032px breakpoint, we should check with @jarekmorawski to see what breakpoint we should use (960px feels right to me, and is the breakpoint where WP collapses the admin sidebar).

mattsherman · 2022-11-12T01:36:36Z

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

 					>
 						<InputControl
 							{ ...regularPriceProps }
+							className="woocommerce-field-width"


I'd put this in currencyInputProps above. That way, it will get merged I believe, and not just override any other classes already set on the component via regularPriceProps or anything else up the chain.

Adding it to currencyInputProps is not yielding the expected result. It seems to be added to multiple elements, resulting in a 25% width

I think we need a way of adding it only to the InputControl. Do you know a better way?

Ah, wait... we are wrapping the InputControl in a BaseControl, and we are pulling out className to set on the BaseControl. So, I'd still suggest putting it in currencyInputProps, but only set it on either BaseControl or InputControl... not sure which one would be best, probably InputControl, since the label or help could be longer (as your screenshot shows).

Honestly, with what we are doing here with BaseControl and InputControl, we ideally should have a new component such as CurrencyInput that takes care of all of this. Otherwise, things are quickly going to get messy and difficult to maintain.

mattsherman · 2022-11-12T01:36:43Z

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

 					>
 						<InputControl
 							{ ...salePriceProps }
+							className="woocommerce-field-width"


Same as above.

mattsherman · 2022-11-12T01:37:22Z

plugins/woocommerce/changelog/update-pricing-inputs-width

+Significance: patch
+Type: enhancement
+
+Adapt the width of pricing fields in new product management experience


I'd probably make this comment a little more specific. "Adapt" feels a bit vague.

mattsherman · 2022-11-12T01:41:05Z

@nathanss I notice the "Run lint checks" was canceled. Not really sure (sometimes that just happens, I think). You can re-run checks by clicking "Details" on the failing check and click the "Re-run jobs" button. Of course, pushing new commits will also trigger a re-run, so you don't really need to worry about this until you are at the state where you are looking for a "final" review!

…akpoint 960px mixin

nathanss · 2022-11-14T13:38:35Z

@mattsherman thanks for the mixin suggestions. Never used those before!

mattsherman · 2022-11-14T22:19:16Z

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

 					>
 						<InputControl
 							{ ...regularPriceProps }
+							className="woocommerce-field-width"


Ah, wait... we are wrapping the InputControl in a BaseControl, and we are pulling out className to set on the BaseControl. So, I'd still suggest putting it in currencyInputProps, but only set it on either BaseControl or InputControl... not sure which one would be best, probably InputControl, since the label or help could be longer (as your screenshot shows).

Honestly, with what we are doing here with BaseControl and InputControl, we ideally should have a new component such as CurrencyInput that takes care of all of this. Otherwise, things are quickly going to get messy and difficult to maintain.

… it manually for the two currency fields

Sent as a parameter to getInputProps and getSelectControlProps to avoid overwriting any additional className

mattsherman

Just a minor changelog update.

Other than that, looks good and tests well.

mattsherman · 2022-11-17T16:28:22Z

plugins/woocommerce/changelog/update-pricing-inputs-width

+Significance: patch
+Type: enhancement
+
+Change the width of pricing fields to 50% in big screens (>960px) in new product management experience


This should be updated since it is not just the pricing fields that are being updated.

mattsherman · 2022-11-17T16:36:02Z

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

 				<CardBody>
 					<BaseControl
 						id="product_pricing_regular_price"
-						className={ regularPriceProps?.className ?? '' }


I'm okay with this change. As mentioned before, ideally we should abstract away this <BaseControl> usage so we aren't directly using it here. That is for the future though.

mattsherman

Thanks for updating the changelog. Looks good. Nice work!

nathanss added 2 commits November 10, 2022 15:59

Add css class to change the input width based on the screen's width

03ab060

Changelog

3008923

github-actions bot added focus: react admin plugin: woocommerce Issues related to the WooCommerce Core plugin. labels Nov 10, 2022

mattsherman suggested changes Nov 12, 2022

View reviewed changes

nathanss added 3 commits November 14, 2022 10:17

Move class from pricing-section.scss to product-page.scss and use bre…

ee9d39d

…akpoint 960px mixin

Move className from currencyInputProps to InputControl

5af59f5

Improve changelog

e3a4623

nathanss requested a review from mattsherman November 14, 2022 17:22

mattsherman suggested changes Nov 14, 2022

View reviewed changes

Move 'half-width-field' class to currencyInputProps instead of adding…

dd838b3

… it manually for the two currency fields

nathanss requested a review from mattsherman November 15, 2022 13:30

Add 'half-width-field' class to SKU and Shipping Class

4190c4d

Sent as a parameter to getInputProps and getSelectControlProps to avoid overwriting any additional className

mattsherman suggested changes Nov 17, 2022

View reviewed changes

Update changelog

db05dc2

nathanss requested a review from mattsherman November 18, 2022 14:17

mattsherman approved these changes Nov 18, 2022

View reviewed changes

nathanss merged commit 6d4c1b3 into trunk Nov 18, 2022

nathanss deleted the update/pricing-inputs-width branch November 18, 2022 16:03

github-actions bot added this to the 7.3.0 milestone Nov 18, 2022

github-actions bot mentioned this pull request Jan 13, 2023

Test WooCommerce 7.3 UVLabs/location-picker-at-checkout-for-woocommerce#59

Closed

Adapt the width of the pricing fields #35545

Adapt the width of the pricing fields #35545

Uh oh!

Conversation

nathanss commented Nov 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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 Nov 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test Results Summary

Uh oh!

mattsherman 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!

mattsherman commented Nov 12, 2022

Uh oh!

nathanss commented Nov 14, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattsherman 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!

mattsherman left a comment • 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

nathanss commented Nov 10, 2022 •

edited

Loading

github-actions bot commented Nov 10, 2022 •

edited

Loading

mattsherman left a comment •

edited

Loading