Create attribute within the new product MVP #35100

louwie17 · 2022-10-14T14:26:19Z

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:

This allows users to create a new attribute if the name they are searching for doesn't exist within the attribute list.
This is currently dependent on #34999 to be merged, which is why it's pointing at that branch.

Kapture.2022-10-14.at.11.23.36.mp4

Address the creation part in #34331 .

How to test the changes in this Pull Request:

Load this branch, build it, and make sure you have the new-product-management-experience feature flag enabled (you can do so using the latest version of the Beta tester within the mono repo).
Go to Products > Add new (MVP) and fill out some of the initial details like name and price
Scroll down and click Add first attribute in the attributes section
Now write a random name in the attribute search field in the table, of an attribute that doesn't exist.
The 'Create "your new attribute name"' should show as an option
Click that and the new item should show as selected and the term field should focus
With localStorage.setItem( 'debug', 'wc-admin:*' ); run prior, it should show that a product_attribute_add_custom_attribute has been fired.
Start adding attribute terms, this dropdown should initial be blank and any text you put in will show up as a Create "text" option, allowing you to add several new items.
Click Add the custom attribute should show correctly in the main list, now hit Update or Publish
Go to old product editor and load the same product you just saved, check the attributes tab and make sure the custom one's show up correctly.

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.

louwie17 · 2022-10-14T14:40:18Z

...ins/woocommerce-admin/client/products/fields/attribute-input-field/attribute-input-field.tsx

 		: null;

 	return (
-		<SelectControl< Pick< QueryProductAttribute, 'id' | 'name' > >


This is just indented as I had to add a Fragment around this in order to add the create modal

github-actions · 2022-10-14T14:47:07Z

Test Results Summary

Commit SHA: 9a7f07b

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

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

Very nice work, Lourens! Testing well on my end; left a few minor comments, but nothing blocking and pre-approving this PR.

Not a result of this PR, but I noticed a couple of oddities around styling:

The modal appears off when editing an existing attribute

The loading indicator is slightly disorienting and jumpy, making it challenging to click an item while loading occurs. This loading indicator might be better as the select control's icon instead of in the list (cc @jarekmorawski). Though this may already be planned and blocked by the select control icon issue.

joshuatf · 2022-11-16T15:31:16Z

plugins/woocommerce-admin/client/products/fields/attribute-field/add-attribute-modal.tsx

 };

+function isEmptyItem(
+	item: HydratedAttributeType | { id: undefined }


Could this be a partial of HydratedAttributeType or does it need to have a separate { id: undefined }?

I wanted to have a clear differentiator between when a attribute was set and when it wasn't. Using a Partial of HydratedAttributeType would make that a bit more difficult and we did have to manually check if all the attributes exist on that property to deem it as the HydratedAttributeType in several places.

I could potentially just try null, let me see if that works well.

I updated it to using HydratedAttributeType | null so that things are more clear.

joshuatf · 2022-11-16T15:35:15Z

plugins/woocommerce-admin/client/products/fields/attribute-field/add-attribute-modal.tsx

-																		options:
-																			undefined,
-																	}
+																	typeof val ===


It would be great to have this as a util method so we could also test it.

E.g., parseAttribute( val: string | Attribute )

Good suggestion, I did do this as part of the fd8af0d commit (it includes the other changes as well)

joshuatf · 2022-11-16T15:38:57Z

...ins/woocommerce-admin/client/products/fields/attribute-input-field/attribute-input-field.tsx

-					name: attribute.name,
-					options: [],
-				} );
+				if ( attribute.id === -99 ) {


To reduce the number of attribute.id === -99 checks, maybe we could create a util method isNewAttribute( attribute ) that handles this check for us.

Done as part of: 2dcbcd5

joshuatf · 2022-11-16T15:41:49Z

plugins/woocommerce-admin/client/products/fields/attribute-field/add-attribute-modal.tsx

-																		index +
-																		'].terms',
+														{ isEmptyItem(
+															formAttr


I know that formAttr was not added in this PR, but this confused me for a minute as I thought it was a "form attribute." Would attribute be a more accurate name? Or is there something special about these that is related to the form?

jarekmorawski · 2022-11-16T15:54:08Z

The loading indicator is slightly disorienting and jumpy, making it challenging to click an item while loading occurs. This loading indicator might be better as the select control's icon instead of in the list (cc @jarekmorawski). Though this may already be planned and blocked by the select control icon issue.

Definitely. The latest approach is to replace the search icon with a loading indicator, but we'll work on that as part of another issue.

louwie17 · 2022-11-17T10:48:51Z

@joshuatf this should be ready for re-review, I addressed all your changes.
Also in relation to the two issues you found:

The modal appears off when editing an existing attribute

This should be fixed now after the rebase as I fixed this as part of the create attribute PR -> #35132 (at-least I can't reproduce it anymore)

The loading indicator is slightly disorienting and jumpy, making it challenging to click an item while loading occurs. This loading indicator might be better as the select control's icon instead of in the list (cc @jarekmorawski). Though this may already be planned and blocked by the select control icon issue.

As @jarekmorawski mentioned this is blocked by the other issue in flight to allow us to move the loading indicator within the input field.

joshuatf

Thanks for the changes, @louwie17! This is still testing well for me and changes look great.

Left one more comment about using refs instead of dom selectors for a possible follow-up issue. Otherwise, LGTM! 🚢

joshuatf · 2022-11-18T17:57:10Z

...ins/woocommerce-admin/client/products/fields/attribute-input-field/attribute-input-field.tsx

 									getItemProps={ getItemProps }
 								>
-									{ item.name }
+									{ item.id === -99 ? (


This could also use the new isNewAttributeListItem right?

Nice catch, not sure how I missed that, fixed here: 9a7f07b

joshuatf · 2022-11-18T18:00:07Z

plugins/woocommerce-admin/client/products/fields/attribute-field/add-attribute-modal.tsx

-			setTimeout( () => {
+		setTimeout( () => {
+			const valueInputField: HTMLInputElement | null =
+				document.querySelector(


I think ideally we could use a ref here instead of dom selectors, but we can save that for a follow-up issue as I know that wasn't really a part of this PR.

Yeah currently the SelectControl component doesn't allow us to pass in a ref, I could definitely add that though as a separate PR

joshuatf · 2022-11-18T18:02:54Z

plugins/woocommerce-admin/client/products/fields/attribute-field/add-attribute-modal.tsx

 };

+function isEmptyItem(
+	item: HydratedAttributeType | { id: undefined }


…d: undefined

louwie17 · 2022-11-21T12:59:37Z

@joshuatf could I get a re-review/approval I just updated the one last spot that needed to use the helper function.

joshuatf

Changes look good! LGTM

github-actions bot added focus: react admin release: highlight Issues that have a high user impact and need to be discussed/paid attention to. labels Oct 14, 2022

louwie17 commented Oct 14, 2022

View reviewed changes

louwie17 marked this pull request as ready for review October 17, 2022 16:23

louwie17 requested a review from a team October 17, 2022 16:23

louwie17 force-pushed the add/34331_add_attributes_modal branch from 8d5ce35 to 7420a10 Compare October 18, 2022 20:22

Base automatically changed from add/34331_add_attributes_modal to trunk October 19, 2022 19:28

louwie17 force-pushed the add/34331_create_attribute branch from 9f67bfd to 24d1302 Compare October 20, 2022 12:19

github-actions bot added plugin: woocommerce Issues related to the WooCommerce Core plugin. and removed release: highlight Issues that have a high user impact and need to be discussed/paid attention to. labels Oct 20, 2022

louwie17 force-pushed the add/34331_create_attribute branch from 24d1302 to a27aec1 Compare November 1, 2022 18:59

AnnaMag added the priority: high label Nov 4, 2022

louwie17 force-pushed the add/34331_create_attribute branch from 02a8b3f to db5bea2 Compare November 15, 2022 14:34

joshuatf previously approved these changes Nov 16, 2022

View reviewed changes

louwie17 dismissed joshuatf’s stale review via fd8af0d November 17, 2022 10:39

louwie17 force-pushed the add/34331_create_attribute branch from 521b1ac to fd8af0d Compare November 17, 2022 10:39

louwie17 requested a review from joshuatf November 17, 2022 12:27

joshuatf previously approved these changes Nov 18, 2022

View reviewed changes

louwie17 added 9 commits November 21, 2022 13:56

Add initial add new option

509c60d

Hook in create attribute modal to add attribute field

ff61fae

Add unit tests for the create attribute modal

91562ea

Add extra test to attribute input field

2f73045

Add changelog

e7a7074

Add custom attribute term input field and support for custom attributes

72e82da

Fix tets

c61eba3

Add track for custom attribute creation

03e035a

Fix changes after merge conflict

d1e38ee

louwie17 added 5 commits November 21, 2022 13:56

Revert one change

44546dd

Fix lint error

1827795

Seperate out some logic and make use of null as empty object versus i…

264ba27

…d: undefined

Add isNewAttributeListItem helper function

bc11084

Make use of helper function for create new markup

9a7f07b

louwie17 dismissed joshuatf’s stale review via 9a7f07b November 21, 2022 12:58

louwie17 force-pushed the add/34331_create_attribute branch from 2dcbcd5 to 9a7f07b Compare November 21, 2022 12:58

louwie17 requested a review from joshuatf November 21, 2022 12:59

joshuatf approved these changes Nov 21, 2022

View reviewed changes

louwie17 merged commit 7ec3210 into trunk Nov 21, 2022

louwie17 deleted the add/34331_create_attribute branch November 21, 2022 14:56

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

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

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

Closed

Create attribute within the new product MVP #35100

Create attribute within the new product MVP #35100

Uh oh!

Conversation

louwie17 commented Oct 14, 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!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Test Results Summary

Uh oh!

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

jarekmorawski commented Nov 16, 2022

Uh oh!

louwie17 commented Nov 17, 2022

Uh oh!

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

louwie17 commented Nov 21, 2022

Uh oh!

joshuatf left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

louwie17 commented Oct 14, 2022 •

edited

Loading

github-actions bot commented Oct 14, 2022 •

edited

Loading