Enhance getInputProps to allow passing of non-overridden props #35034

joshuatf · 2022-10-11T14:53:03Z

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:

Previously, adding an onChange handler or anything else that collided with getInputProps would require recalling getInputProps.someFunction inside the new handler. This PR introduces a second argument to getInputProps which passes props to the control without overriding the original handlers where possible.

Some props will not override the original handler (e.g., if you pass on onChange it will be combined with the Form component's onChange) and some will override (e.g., value can not be combined and the consumer prop will override the original value).

This PR also uncovered numerous TS issues, so a getCheckboxProps was introduced into the Form component to omit invalid props (e.g., value for checkboxes). More specific prop getters are probably in order (getSelectProps), but I think we can iterate on these as we go.

Finally, a sanitize method was added as part of the optional consumer input props. This is a shortcut to sanitizing the input value and handling changes on blur.

TL;DR:

getInputProps now takes a second argument to combine/override props where necessary
sanitize method can be added to an input to handle sanitization on blur
getCheckboxProps was introduced for props specific to checkboxes

Closes #34996 .

How to test the changes in this Pull Request:

Navigate to the new product page Products -> Add new (MVP)
Check that styling for checkboxes is still correct
Add values to the price and dimension fields, using thousand commas and decimal separators (depending your locale)
Make sure they are sanitized on blur and persist correctly on save

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 --filter=<project> run 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-10-11T14:59:13Z

Test Results Summary

Commit SHA: 6519e81

Test 🧪	Passed ✅	Failed 🚨	Broken 🚧	Skipped ⏭️	Unknown ❔	Total 📊	Duration ⏱️
API Tests	196	0	0	2	0	198	0m 46s
E2E Tests	186	0	0	3	0	189	15m 52s

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.

joshuatf

Just noting that we have more upcoming work (I also have an open PR) that will benefit from being able to pass in additional handlers without overriding the Form's handlers.

joshuatf · 2022-10-11T14:56:46Z

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

-								regularPriceProps?.onChange( sanitizedValue );
-							} }
+							value={ formatCurrencyDisplayValue( regularPriceProps?.value, currencyConfig, formatAmount ) }
+							onBlur={ () => setValue( 'regular_price', sanitizePrice( regularPriceProps.value ) ) }


@octaedro I moved the sanitization to onBlur. I think this is a little bit more conventional as I was worried about input being sanitized as it was typed could lead to odd behavior.

But please let me know if I broke anything here or if this needs to be sanitized as we type.

Sounds good to me! Good idea!

joshuatf · 2022-10-11T14:59:38Z

plugins/woocommerce-admin/client/products/sections/utils.ts

+	const currencyPosition = symbolPosition.includes( 'left' )
+		? 'prefix'
+		: 'suffix';
+
 	return {


My goal was to unwrap some of these so that we can:

More easily follow where props are being passed

Reduce the knowledge of form handlers in this function

Reduce overall logic in this function

joshuatf · 2022-10-13T21:17:32Z

@octaedro Thanks for the ping on rebasing this one in Slack! After rebasing, a lot of TS issues cropped up that I think we were able to errantly bypass before.

I added a getCheckboxProps method to Form and also introduced a sanitize method as a shortcut for inputs. I updated the description above to reflect all the changes.

@mdperez86 since this also affects a lot of your recent additions around dimensions, it would be great to get your 👀 on this as well.

joshuatf · 2022-10-13T21:19:36Z

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

 		};
 	}

+	function getCheckboxProps< Value = Values[ keyof Values ] >(


Should this be getCheckboxInputProps or getCheckboxControlProps instead? The reason I ask is we'll probably want to introduce a getSelectProps which sounds odd and could be interpreted the wrong way. Also okay leaving it as is, but wanted to get some feedback.

I would lean towards getCheckboxControlProps. But I'm also ok leaving it as it is now.

I think that makes sense too since we're really creating props specifically for our controls 👍

octaedro · 2022-10-19T13:56:29Z

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

+							{ ...getInputProps( 'name', {
+								onBlur: setSkuIfEmpty,
+							} ) }


octaedro

Nice job @joshuatf! This is testing well here and the code looks good.
There is a small visual error with the Feature this product checkbox, the rest looks great!

joshuatf · 2022-10-19T19:53:46Z

There is a small visual error with the Feature this product checkbox, the rest looks great!

Good catch @octaedro! Updated in 38f80d8

Also gave this a rebase; could you take a look when you have time?

octaedro

Thank you @joshuatf for addressing the changes! The initialValues issue was a pretty tricky thing to deal with.
Good job!

octaedro

🚀

github-actions · 2022-10-21T14:04:02Z

Hi @joshuatf, 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

joshuatf requested a review from a team October 11, 2022 14:53

joshuatf self-assigned this Oct 11, 2022

github-actions bot added focus: react admin package: @woocommerce/components issues related to @woocommerce/components plugin: woocommerce Issues related to the WooCommerce Core plugin. labels Oct 11, 2022

joshuatf commented Oct 11, 2022

View reviewed changes

joshuatf force-pushed the update/34996 branch 2 times, most recently from 6dd3ccc to 9298538 Compare October 13, 2022 21:10

joshuatf commented Oct 13, 2022

View reviewed changes

joshuatf mentioned this pull request Oct 17, 2022

[Components] Form component should have utility methods for various input types #35138

Open

3 tasks

octaedro reviewed Oct 19, 2022

View reviewed changes

joshuatf force-pushed the update/34996 branch from 6210905 to ad7e1b1 Compare October 19, 2022 19:41

github-actions bot added package: @woocommerce/data issues related to @woocommerce/data release: highlight Issues that have a high user impact and need to be discussed/paid attention to. labels Oct 19, 2022

octaedro previously approved these changes Oct 20, 2022

View reviewed changes

joshuatf added 11 commits October 20, 2022 14:25

Allow additional props to be passed to the Form getInputProps method

05e8df7

Remove getTextControlProps

a70e027

Pass additional shared props through getInputProps in shipping

942d1a8

Simplify checkbox props

1c6895f

Unwrap currency props

26ffa53

Use onBlur event to sanitize prices

fc6db75

Add changelog entry

cd7f621

Add option to get checkbox props to form context helpers

cc50efa

Update checkbox tracks handler naming and typing

5eca144

Fix up usage of getInputProps

d07877d

Add helper sanitize method

ec5c2b1

joshuatf added 11 commits October 20, 2022 14:25

Use sanitize helper method for product input fields

b9781fe

Fix inventory input props after rebase

9d4148f

Fix shipping typo

1954205

Fix up form types after rebase

2c7d393

Align all checkboxes on product page

9fcefb9

Rename new checkbox helper to getCheckboxControlProps

fa4d3b7

Add helper method to get select control props

f8fe8ab

Add data changelog entry

d03965d

Check for product name length on blur

3a6a752

Add initial value for name to prevent uncontrolled value

71ad30c

Add initial value for sku

6519e81

joshuatf dismissed octaedro’s stale review via 6519e81 October 20, 2022 21:31

joshuatf force-pushed the update/34996 branch from e03cc19 to 6519e81 Compare October 20, 2022 21:31

github-actions bot removed the release: highlight Issues that have a high user impact and need to be discussed/paid attention to. label Oct 20, 2022

octaedro approved these changes Oct 21, 2022

View reviewed changes

joshuatf merged commit 28f8e7f into trunk Oct 21, 2022

joshuatf deleted the update/34996 branch October 21, 2022 14:03

github-actions bot added this to the 7.2.0 milestone Oct 21, 2022

github-actions bot mentioned this pull request Dec 15, 2022

Test WooCommerce 7.2 UVLabs/location-picker-at-checkout-for-woocommerce#57

Closed

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

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

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enhance getInputProps to allow passing of non-overridden props #35034

Enhance getInputProps to allow passing of non-overridden props #35034

joshuatf commented Oct 11, 2022 •

edited

github-actions bot commented Oct 11, 2022 •

edited

joshuatf left a comment

joshuatf Oct 11, 2022

octaedro Oct 12, 2022

joshuatf Oct 11, 2022

joshuatf commented Oct 13, 2022

joshuatf Oct 13, 2022

octaedro Oct 19, 2022

joshuatf Oct 19, 2022

octaedro Oct 19, 2022

octaedro left a comment

joshuatf commented Oct 19, 2022

octaedro left a comment

octaedro left a comment

github-actions bot commented Oct 21, 2022

Enhance getInputProps to allow passing of non-overridden props #35034

Enhance getInputProps to allow passing of non-overridden props #35034

Conversation

joshuatf commented Oct 11, 2022 • edited

All Submissions:

Changes proposed in this Pull Request:

How to test the changes in this Pull Request:

Other information:

FOR PR REVIEWER ONLY:

github-actions bot commented Oct 11, 2022 • edited

Test Results Summary

joshuatf left a comment

Choose a reason for hiding this comment

joshuatf Oct 11, 2022

Choose a reason for hiding this comment

octaedro Oct 12, 2022

Choose a reason for hiding this comment

joshuatf Oct 11, 2022

Choose a reason for hiding this comment

joshuatf commented Oct 13, 2022

joshuatf Oct 13, 2022

Choose a reason for hiding this comment

octaedro Oct 19, 2022

Choose a reason for hiding this comment

joshuatf Oct 19, 2022

Choose a reason for hiding this comment

octaedro Oct 19, 2022

Choose a reason for hiding this comment

octaedro left a comment

Choose a reason for hiding this comment

joshuatf commented Oct 19, 2022

octaedro left a comment

Choose a reason for hiding this comment

octaedro left a comment

Choose a reason for hiding this comment

github-actions bot commented Oct 21, 2022

joshuatf commented Oct 11, 2022 •

edited

github-actions bot commented Oct 11, 2022 •

edited