New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add/42 new product page #34115
Add/42 new product page #34115
Conversation
Test Results SummaryCommit SHA: f430d0e
To view the full E2E test report, click here. To view all test reports, visit the WooCommerce Test Reports Dashboard. |
@octaedro @louwie17 I think that we need to either:
We can’t expect people to make code changes to toggle the feature flag, as that makes it way less easy for non-devs to do, and makes it cumbersome for devs. |
Hey @mattsherman, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks great @octaedro and it tested well.
One big thing is that the feature flag in core.json
should be false
as otherwise it is enabled for the next release, keeping it enabled in development.json
is fine I think.
I did leave some other comments through out.
I also think it's fine we don't have the Feature toggle under advance settings yet, I think we shouldn't add that until we have a working version ( when our 0.5 MVP is done).
@@ -0,0 +1,16 @@ | |||
/** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The file name should follow the component name, so maybe rename the file to add-product.tsx
Or to be more explicit about being a page, you could call it AddProductPage
and add-product-page.tsx
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I fixed this in commit fbc4f44.
@@ -15,6 +15,7 @@ | |||
"minified-js": false, | |||
"mobile-app-banner": true, | |||
"navigation": true, | |||
"new-product-management-experience": true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We want to set this to false
, as this would mean that once this is merged the next release will have this feature enabled by default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice catch!
@@ -15,6 +15,7 @@ | |||
"minified-js": true, | |||
"mobile-app-banner": true, | |||
"navigation": true, | |||
"new-product-management-experience": true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having this as true
is fine for now I think, given we have the MVP button anyway.
Once the MVP button is gone, we might want to turn this back to false and have everyone enabled it themselves.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We usually leave the feature flags turned on in dev since it's a work in progress and the dev environment is exactly to work on those things. I'm not opposed to turning the flag off, but if we follow that pattern, I do think that we should turn off all of them in dev.
@@ -1,93 +1,134 @@ | |||
/* global woocommerce_admin */ | |||
( function( $, woocommerce_admin ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you make any changes in this file? or is it only formatting?
If only formatting I think you are better off reverting this change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you make any changes in this file? or is it only formatting?
If only formatting I think you are better off reverting this change.
Nope, it's only formatting. Sounds good to me. I reverted the formatting fix in commit 53fae30
@@ -213,6 +213,17 @@ public static function get_items() { | |||
); | |||
} | |||
|
|||
$add_product_mbp = array(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Kinda curious what mbp
stands for? did you mean mvp
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good eyes! I fixed the typo in the commit f430d0e.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great @octaedro, thanks for the updated changes this tested well and the code looks great, nice work!!
Hi @octaedro, thanks for merging this pull request. Please take a look at these follow-up tasks you may need to perform:
|
All Submissions:
Changes proposed in this Pull Request:
This PR adds the code to:
new-product-management-experience
).Products > Add New
(using the old and the new navigation) and theAdd New
button underProducts > All Products
.The
Add manually
in theAdd products
task.Closes this issue (42-gh-woocommerce/mothra-private).
UPDATE
Since there is no button to create a product with another type other than simple, the commit 2953fb1 has been created to add a menu item named
Products
>Add New (MVP)
and remove (temporarily) the URL override of the old buttons.After having the button mentioned above in place, the commit 2953fb1 should be deleted.
How to test the changes in this Pull Request:
new-product-management-experience
underTools > WCA Test Helper > Features
.Add product
after pressing the following buttons:Products > Add New
and theAdd New
button underProducts > All Products
and theAdd manually
in theAdd my products
task.view_new_product_management_experience
is being recorded when viewing theAdd product
page.Add New
button works as expected.Other information:
pnpm changelog add --filter=<project>
?FOR PR REVIEWER ONLY: