Add a product header component to the blocks interface #37152

mdperez86 · 2023-03-09T19:30:27Z

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:

Closes #37005

How to test the changes in this Pull Request:

Have you followed the Writing high-quality testing instructions guide?

Go to /wp-admin/admin.php?page=wc-admin&path=/add-product a save button should be shown in the top-right corner of the screen.

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?
Have you included testing instructions?

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.

joshuatf

I think this one might need a rebase to remove the non-header related changes.

joshuatf · 2023-03-09T23:16:07Z

packages/js/product-editor/src/components/header/header.tsx

+			const { isPostSavingLocked } = select( 'core/editor' );
+			return {
+				// eslint-disable-next-line @typescript-eslint/ban-ts-comment
+				// @ts-ignore


I think I needed these during testing since it did not recognize these properties, but might be good to add a quick comment to the above line for why we need to ignore.

After rebasing this was not longer needed

joshuatf · 2023-03-09T23:16:14Z

packages/js/product-editor/src/components/header/header.tsx

+				// @ts-ignore
+				isProductLocked: isPostSavingLocked(),
+				// eslint-disable-next-line @typescript-eslint/ban-ts-comment
+				// @ts-ignore


Same comment here.

After rebasing this was not longer needed

joshuatf · 2023-03-09T23:16:43Z

packages/js/product-editor/src/components/header/header.tsx

+	const { saveEditedEntityRecord } = useDispatch( 'core' );
+
+	function handleSave() {
+		saveEditedEntityRecord( 'postType', 'product', product?.id );


Optional, but wondering if we will always have the product or need to use ? to check its existence.

Done -> 3686408

mdperez86 · 2023-03-13T13:33:23Z

I think this one might need a rebase to remove the non-header related changes.

Yes, that's why this PR is still In Progress status. This depends on #37132.

github-actions · 2023-03-13T13:54:27Z

Test Results Summary

Commit SHA: 1d0165e

Test 🧪	Passed ✅	Failed 🚨	Broken 🚧	Skipped ⏭️	Unknown ❔	Total 📊	Duration ⏱️
API Tests	259	0	0	2	0	261	0m 47s
E2E Tests	191	0	0	10	0	201	13m 58s

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.

codecov · 2023-03-13T18:19:43Z

Codecov Report

Merging #37152 (1d0165e) into trunk (cbafbd5) will increase coverage by 0.0%.
The diff coverage is 66.7%.

Additional details and impacted files

@@           Coverage Diff            @@
##             trunk   #37152   +/-   ##
========================================
  Coverage     46.7%    46.7%           
  Complexity   17197    17197           
========================================
  Files          429      429           
  Lines        64884    64884           
========================================
+ Hits         30287    30289    +2     
+ Misses       34597    34595    -2

Impacted Files	Coverage Δ
plugins/woocommerce/includes/class-wc-install.php	`69.0% <66.7%> (ø)`

... and 1 file with indirect coverage changes

louwie17

I am inclined to approve this as is, as it does work, although I do think we should make better use of the available editor selectors, especially around the ones that are already aware what product we are editing.
Reason for this is, is that all the selectors will be memoized, and things would be far more optimized using specialized selectors instead of prop drilling an entire product object down.

This also falls inline with all of Gutenberg's current approach, especially around these components.

louwie17 · 2023-03-14T14:52:16Z

packages/js/product-editor/src/components/editor/editor.tsx

-							header={ <Header title={ product.name } /> }
+							header={
+								<Header
+									product={ product }


I think we should prevent prop drilling the product object down and instead make use of the available data store selectors for this.
For example, you can get the active product by calling select( 'core/editor' ).getCurrentPost()
Although I did realize this is currently not working as expected as product is being passed in as an empty object, if you add this additional condition || ! product.id here:

woocommerce/plugins/woocommerce-admin/client/products/product-page.tsx

Line 25 in 3ba7834

if ( ! product ) {

It should fix this.

done -> b34cd1d

louwie17 · 2023-03-14T14:53:09Z

packages/js/product-editor/src/components/editor/editor.tsx

+									title={
+										product.name ||
+										__( 'Add new product', 'woocommerce' )
+									}


We can probably do something similar for title and make use of the getEditedPostAttribute selector, the Gutenberg TemplateTitle does something similar.

When in product creation, if we use getEditedPostAttribute to get the name of the product to use it as the title of the header then we will have AUTO_DRAFT which is the default value set for the name. We want the title Add new product when creating the product right @jarekmorawski ?

That's correct. The default title is Add new product. It changes to the product's name after the user adds it.

louwie17 · 2023-03-14T14:57:25Z

packages/js/product-editor/src/components/header/header.tsx

+			const { isPostSavingLocked } = select( 'core/editor' );
+			return {
+				isProductLocked: isPostSavingLocked(),
+				isSaving: isSavingEntityRecord(


Instead of using isSavingEntityRecord with the product id you can use isCurrentPostPending I believe, or isSavingPost.

done -> b34cd1d

According to what we discussed I reverted this back to what we had previously because using functions from core/editor are very tied to the Post structure and the saving process does not work because of that. See -> 62efff6

louwie17 · 2023-03-14T14:58:12Z

packages/js/product-editor/src/components/header/header.tsx

+	const { saveEditedEntityRecord } = useDispatch( 'core' );
+
+	function handleSave() {
+		saveEditedEntityRecord( 'postType', 'product', product.id );


The editor store also has a savePost action which should automatically use the active one.

done -> b34cd1d

louwie17

Looking good @mdperez86
Just left one more comment about removing the product prop from the Header component now.

louwie17 · 2023-03-15T10:19:23Z

packages/js/product-editor/src/components/header/header.tsx

+				isSaving: isSavingPost(),
+			};
+		},
+		[ product.id ]


We can remove this now right? and in turn also remove the product prop from this component?
I believe the core/editor should re-trigger this useSelect if the active post gets updated, so listening to the id won't be necessary.

Ohh! right! Thanks

Done -> 193d285 Thanks. Good catch.

joshuatf · 2023-03-15T17:55:19Z

packages/js/product-editor/src/components/header/header.tsx

+					isBusy={ isSaving }
+					disabled={ isDisabled }
+				>
+					{ __( 'Save', 'woocommerce' ) }


I'm seeing in the new design that this is Add when the product has not yet been created yet (auto-draft). Okay with keeping this for a follow-up though if you prefer.

done here -> d28c40b Thanks @joshuatf

louwie17 · 2023-03-16T14:47:23Z

packages/js/product-editor/src/components/editor/editor.tsx

+							header={
+								<Header
+									productId={ product.id }
+									title={ product.name }


Just a note here that when the product is new the name doesn't get updated.
I assume this because we are not passing in the the product from a useSelect hook in the product-page but we are for existing products.
I think we probably don't want to do that anyway, and only pass the product in a constant and for the rest rely on the selectors within.

louwie17

LGTM 🚀
This works well as is, although we should create follow up issues for the redirection of creating a brand new product and fixing the issue around state updates between existing and new products.

louwie17 · 2023-03-16T14:48:57Z

packages/js/product-editor/src/components/header/header.tsx

+	const { saveEditedEntityRecord } = useDispatch( 'core' );
+
+	function handleSave() {
+		saveEditedEntityRecord( 'postType', 'product', productId );


We should also re-direct to the new product form when saving a new post for the first time.
But I do realize that might of been out of scope for this issue.

see -> 64df88c
see -> 0665be4

joshuatf

This looks good on my end. One possible change going forward, but don't want it to block this PR.

I'll let @louwie17 give the final approval on this.

joshuatf · 2023-03-16T17:53:56Z

packages/js/product-editor/src/components/header/header.tsx

+	const isProductNameDirty = product.name !== productName;
+	const isCreating = productName === AUTO_DRAFT_NAME;
+
+	let title = '';


Not a necessity in this PR since this is already pushing the original scope, but nice to have this as a util with some tests like we did before - https://github.com/woocommerce/woocommerce/blob/trunk/packages/js/product-editor/src/utils/get-product-title.ts#L16

But we can handle this in a follow-up.

And what is the name you suggest here for that util function? We already have get-product-title. It would be great if we could reuse the same one but it has a different behavior. Do you like this one get-header-title for the new function?

done -> 1d0165e

I did not add tests to that function now because testing are not configured for the current product-editor package yet. That configuration is been managed in other PR.

louwie17

Just left one final suggestion 🙈

Tested very well though :)

louwie17 · 2023-03-17T10:00:02Z

packages/js/product-editor/src/components/header/header.tsx

+					'product',
+					productId
+				),
+				product: getEditedEntityRecord(


Did you actually want to return productName only here, instead of the entire product.
Sorry for harping on this, but return the product would trigger a re-render on every product update, although we only care about the name.

Should I use a different function for that, because I could not find it within the EntityRecord selectors.
Or do you meant calling the const { name } = getEditedEntityRecord(...) like that and just return the name of the product like { productName: name }?

Yeah that second option.

louwie17 · 2023-03-17T10:02:19Z

packages/js/product-editor/src/components/header/header.tsx

 };

-export function Header( { title }: HeaderProps ) {
+export function Header( { productId, productName }: HeaderProps ) {


To not conflict with the useSelect, maybe we want to convert productName to initialProductName or something like that, or the one within the useSelect could be editedProductName.

louwie17 · 2023-03-17T10:11:27Z

plugins/woocommerce-admin/client/products/product-page.tsx

+					'postType',
+					'product',
+					productId
+				) as Product,


Nice find, getEntityRecord does make more sense here 🎉

… the default title value

louwie17

LGTM 🚀 Nice work @mdperez86, and thanks for addressing all the changes and the back and forth. This is good to merge 🚀

mdperez86 requested a review from a team March 9, 2023 19:30

mdperez86 self-assigned this Mar 9, 2023

github-actions bot added focus: react admin [team:Ghidorah] plugin: woocommerce Issues related to the WooCommerce Core plugin. labels Mar 9, 2023

joshuatf reviewed Mar 9, 2023

View reviewed changes

mdperez86 force-pushed the add/37005 branch from 7dd2ea5 to d0707e2 Compare March 13, 2023 13:23

mdperez86 force-pushed the add/37005 branch from 3686408 to 6f42f6f Compare March 13, 2023 17:40

github-actions bot removed the plugin: woocommerce Issues related to the WooCommerce Core plugin. label Mar 13, 2023

mdperez86 force-pushed the add/37005 branch from 6f42f6f to 8cb3dac Compare March 13, 2023 17:52

mdperez86 force-pushed the add/37005 branch from d9fafa4 to 3ba7834 Compare March 13, 2023 18:47

github-actions bot added the plugin: woocommerce Issues related to the WooCommerce Core plugin. label Mar 13, 2023

mdperez86 requested a review from joshuatf March 14, 2023 13:46

louwie17 reviewed Mar 14, 2023

View reviewed changes

mdperez86 force-pushed the add/37005 branch from 3ba7834 to b34cd1d Compare March 14, 2023 19:32

mdperez86 requested a review from louwie17 March 14, 2023 19:34

louwie17 requested changes Mar 15, 2023

View reviewed changes

mdperez86 force-pushed the add/37005 branch from b34cd1d to 193d285 Compare March 15, 2023 13:45

mdperez86 requested a review from louwie17 March 15, 2023 13:45

mdperez86 force-pushed the add/37005 branch 2 times, most recently from 62efff6 to 6b40ab2 Compare March 15, 2023 16:47

joshuatf reviewed Mar 15, 2023

View reviewed changes

joshuatf mentioned this pull request Mar 15, 2023

Move the Save draft and Preview buttons to the product block header #37249

Closed

3 tasks

mdperez86 force-pushed the add/37005 branch from 6b40ab2 to d28c40b Compare March 16, 2023 13:26

mdperez86 requested a review from joshuatf March 16, 2023 13:28

mdperez86 force-pushed the add/37005 branch from d28c40b to d5f2617 Compare March 16, 2023 14:14

louwie17 reviewed Mar 16, 2023

View reviewed changes

louwie17 previously approved these changes Mar 16, 2023

View reviewed changes

mdperez86 dismissed louwie17’s stale review via d547d1f March 16, 2023 16:01

mdperez86 force-pushed the add/37005 branch from 16e5942 to 0665be4 Compare March 16, 2023 17:13

mdperez86 requested a review from louwie17 March 16, 2023 17:14

joshuatf reviewed Mar 16, 2023

View reviewed changes

louwie17 reviewed Mar 17, 2023

View reviewed changes

mdperez86 added 7 commits March 17, 2023 12:09

Add button and saving logic

9fa949f

Add comment suggestions

e166f91

Add changelog files

9f36a00

Add comment suggestions

efa18bd

Set header title to product name if the name field is dirty otherwise…

c914f17

… the default title value

Navigate to EditProductPage after creating the product succesfully

4f91588

Add get-header-title util and expose edited product name from useSelect

1d0165e

mdperez86 force-pushed the add/37005 branch from 0665be4 to 1d0165e Compare March 17, 2023 15:29

louwie17 approved these changes Mar 17, 2023

View reviewed changes

mdperez86 merged commit e370f25 into trunk Mar 17, 2023
20 checks passed

mdperez86 deleted the add/37005 branch March 17, 2023 16:21

github-actions bot added this to the 7.6.0 milestone Mar 17, 2023

Add a product header component to the blocks interface #37152

Add a product header component to the blocks interface #37152

Conversation

mdperez86 commented Mar 9, 2023 • edited

All Submissions:

Changes proposed in this Pull Request:

How to test the changes in this Pull Request:

Other information:

FOR PR REVIEWER ONLY:

joshuatf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mdperez86 commented Mar 13, 2023

github-actions bot commented Mar 13, 2023 • edited

Test Results Summary

codecov bot commented Mar 13, 2023 • edited

Codecov Report

louwie17 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

louwie17 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

louwie17 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joshuatf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mdperez86 Mar 17, 2023 • edited

Choose a reason for hiding this comment

louwie17 left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

louwie17 left a comment

Choose a reason for hiding this comment

mdperez86 commented Mar 9, 2023 •

edited

github-actions bot commented Mar 13, 2023 •

edited

codecov bot commented Mar 13, 2023 •

edited

mdperez86 Mar 17, 2023 •

edited

louwie17 left a comment •

edited