Skip to content

Add experimental import products task #32835

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.

Sign up for GitHub

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

Jump to bottom
Merged
moon0326 merged 16 commits into from
May 4, 2022
Merged

Add experimental import products task #32835

moon0326 merged 16 commits into from
May 4, 2022

Conversation

@moon0326
Copy link
Contributor

@moon0326 moon0326 commented Apr 29, 2022

All Submissions:

Changes proposed in this Pull Request:

Closes #32788 .

This PR adds experimental import products task that replaces the default Add my product task only when the customers choose Yes to the Currently selling elsewhere? question during the OBW.

How to test the changes in this Pull Request:

  1. Open the plugins/woocommerce/client/admin/config/development.json and change experimental-import-products-task to true
  2. Run pnpm nx dev woocommerce-admin
  3. Start OBW.
  4. Choose Yes, on another platform in the Business Details step.
  5. Navigate to WoocCommerce -> Home and click Add my products
  6. You should see the new Import your products

Screenshots

Collapsed:

Screen Shot 2022-04-29 at 3 57 06 PM

Expanded:
Screen Shot 2022-04-29 at 3 57 10 PM

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 by running pnpm nx affected --target=changelog?

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.

@moon0326 moon0326 requested a review from chihsuan April 29, 2022 23:09
@github-actions github-actions bot added focus: react admin plugin: woocommerce Issues related to the WooCommerce Core plugin. labels Apr 29, 2022
@moon0326 moon0326 requested a review from a team May 2, 2022 19:27
chihsuan
chihsuan requested changes May 3, 2022
View reviewed changes
Copy link
Member

@chihsuan chihsuan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested well. Just have three UI and webpack import issues.

plugins/woocommerce-admin/client/tasks/fills/experimental-import-products/index.tsx Outdated
import { importTypes } from './importTypes';
import './style.scss';
import useProductTypeListItems from '../experimental-products/use-product-types-list-items';
import Stacks from '../experimental-products/stack';
Copy link
Member

@chihsuan chihsuan May 3, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@moon0326 I found Webpack is showing warnings.

[0] Conflicting order. Following module has been added:
[0]  * css ../../node_modules/.pnpm/[email protected][email protected]/node_modules/css-loader/dist/cjs.js!../../node_modules/.pnpm/[email protected]/node_modules/postcss-loader/src/index.js??ruleSet[1].rules[3].use[2]!../../node_modules/.pnpm/[email protected][email protected][email protected]/node_modules/sass-loader/dist/cjs.js??ruleSet[1].rules[3].use[3]!./client/tasks/fills/experimental-import-products/cards.scss
[0] despite it was not able to fulfill desired ordering with these modules:
[0]  * css ../../node_modules/.pnpm/[email protected][email protected]/node_modules/css-loader/dist/cjs.js!../../node_modules/.pnpm/[email protected]/node_modules/postcss-loader/src/index.js??ruleSet[1].rules[3].use[2]!../../node_modules/.pnpm/[email protected][email protected][email protected]/node_modules/sass-loader/dist/cjs.js??ruleSet[1].rules[3].use[3]!./client/tasks/fills/experimental-products/stack.scss
[0]    - couldn't fulfill desired order of chunk group(s)
[0]
[0] webpack 5.70.0 compiled with 1 warning in 10517 ms
[0] No issues found.
[0] assets by status 9.53 MiB [cached] 107 assets
[0] assets by path . 9.13 MiB 116 assets
[0] cached modules 4.44 MiB (javascript) 723 KiB (css/mini-extract) 32.1 KiB (runtime) [cached] 2087 modules
[0]
[0] WARNING in chunk client_tasks_fills_experimental-import-products_CardList_tsx-client_tasks_fills_experimental--c7ef12 [mini-css-extract-plugin]
[0] Conflicting order. Following module has been added:
[0]  * css ../../node_modules/.pnpm/[email protected][email protected]/node_modules/css-loader/dist/cjs.js!../../node_modules/.pnpm/[email protected]/node_modules/postcss-loader/src/index.js??ruleSet[1].rules[3].use[2]!../../node_modules/.pnpm/[email protected][email protected][email protected]/node_modules/sass-loader/dist/cjs.js??ruleSet[1].rules[3].use[3]!./client/tasks/fills/experimental-import-products/cards.scss
[0] despite it was not able to fulfill desired ordering with these modules:
[0]  * css ../../node_modules/.pnpm/[email protected][email protected]/node_modules/css-loader/dist/cjs.js!../../node_modules/.pnpm/[email protected]/node_modules/postcss-loader/src/index.js??ruleSet[1].rules[3].use[2]!../../node_modules/.pnpm/[email protected][email protected][email protected]/node_modules/sass-loader/dist/cjs.js??ruleSet[1].rules[3].use[3]!./client/tasks/fills/experimental-products/stack.scss
[0]    - couldn't fulfill desired order of chunk group(s)

I think we could fix this by reordering Stacks/CardList imports.

import Stacks from '../experimental-products/stack';
import CardList from './CardList';
import { importTypes } from './importTypes';
import './style.scss';
import useProductTypeListItems from '../experimental-products/use-product-types-list-items';

https://stackoverflow.com/questions/60858885/how-do-i-avoid-the-warning-chunk-styles-mini-css-extract-plugin-conflicting-o

The order needs to be consistent everywhere the imports are referenced; otherwise webpack doesn't know how to order them when it creates the chunks.

Copy link
Contributor Author

@moon0326 moon0326 May 3, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the catch! Updated in d0b500a

plugins/woocommerce-admin/client/tasks/fills/experimental-import-products/importTypes.tsx
),
},
{
key: 'from-cart2cart' as const,
Copy link
Member

@chihsuan chihsuan May 3, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For "FROM CART2CART", only the "Learn more" link is clickable. But for "FROM A CSV FILE", the entire card is clickable.

Is this intended?

Screen Recording 2022-05-03 at 11 32 09

Copy link
Member

@chihsuan chihsuan May 3, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cc @verofasulo

Copy link

@verofasulo verofasulo May 3, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For "FROM CART2CART", only the "Learn more" link is clickable. But for "FROM A CSV FILE", the entire card is clickable.

No, both the cards are entirely clickable :) We added the link 'Learn more" after conducting some tests as it reassures the users they are not starting any flow but only collecting information about this option. @chihsuan

Copy link
Contributor Author

@moon0326 moon0326 May 3, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated in 84fe952

plugins/woocommerce-admin/client/tasks/fills/experimental-import-products/CardList.tsx
content: string | JSX.Element;
before: JSX.Element;
};

Copy link
Member

@chihsuan chihsuan May 3, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just noticed that the card title is still black when hovering.

WooCommerce _ Home _ Add Products - Import Version - Hover Effect

Copy link
Contributor Author

@moon0326 moon0326 May 3, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The second screenshot from #32143 shows it as black. Do we need to change the color to blue on hover?

cc @verofasulo

Copy link
Member

@chihsuan chihsuan May 4, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@moon0326 Ah, sorry, I just checked again. It's actually blue now and I think we should change it to black.

Screen Shot 2022-05-04 at 09 45 00

Copy link
Contributor Author

@moon0326 moon0326 May 4, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@chihsuan You're right. I was actually looking at the stack. Updated in 8219510

@chihsuan chihsuan mentioned this pull request May 3, 2022
Merged
8 tasks
chihsuan
chihsuan approved these changes May 4, 2022
View reviewed changes
Copy link
Member

@chihsuan chihsuan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work! @moon0326 Looks good and tested well. 👍 🚀

@moon0326
Copy link
Contributor Author

moon0326 commented May 4, 2022

Looks like two tests are failing with Error: Call to undefined function Automattic\WooCommerce\Internal\Utilities\get_block_template() error. I think they're unrelated to the changes in this PR.

@moon0326 moon0326 merged commit 4a6c330 into trunk May 4, 2022
@moon0326 moon0326 deleted the add/32788-add-experimental-import-products-task-followup branch May 4, 2022 02:13
@github-actions github-actions bot added this to the 6.6.0 milestone May 4, 2022
@github-actions
Copy link
Contributor

github-actions bot commented May 4, 2022

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

@github-actions github-actions bot mentioned this pull request Jun 15, 2022
Closed
@github-actions github-actions bot mentioned this pull request Jul 4, 2022
Closed
This was referenced Jul 5, 2022
Closed
Closed
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@verofasulo verofasulo verofasulo left review comments

@chihsuan chihsuan chihsuan approved these changes

Assignees

No one assigned

Labels

plugin: woocommerce Issues related to the WooCommerce Core plugin.

Projects

None yet

Milestone

6.6.0

Development

Successfully merging this pull request may close these issues.

Add experimental import products task feature & initial components

3 participants

@moon0326 @chihsuan @verofasulo