Skip to content
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

Create @woocommerce/admin-layout package #37094

Merged
merged 28 commits into from Mar 10, 2023
Merged

Conversation

mattsherman
Copy link
Contributor

@mattsherman mattsherman commented Mar 7, 2023

All Submissions:

Changes proposed in this Pull Request:

This adds a new package: @woocommerce/admin-layout with the following initial components moved over from woocommerce/client/admin:

  • WooHeaderItem
  • WooHeaderNavigationItem
  • WooHeaderPageTitle
  • WooFooterItem

More admin layout-specific components will be moved over in the future.

Closes #36721.

How to test the changes in this Pull Request:

  1. Load this branch and run pnpm install && pnpm run build and make sure everything builds correctly.

  2. Smoke test WCAdmin pages and make sure that the headers and footers appears correctly.

    • Headers on all WCAdmin pages
    • CES footer on new product editor page.
  3. Open your console and check if window.wc.adminLayout exists with the following components:

  • WC_FOOTER_SLOT_NAME
  • WC_HEADER_NAVIGATION_SLOT_NAME
  • WC_HEADER_PAGE_TITLE_SLOT_NAME
  • WC_HEADER_SLOT_NAME
  • WooFooterItem
  • WooHeaderItem
  • WooHeaderNavigationItem
  • WooHeaderPageTitle

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.

@mattsherman mattsherman self-assigned this Mar 7, 2023
@github-actions github-actions bot added focus: react admin [team:Ghidorah] package: dependency-extraction-webpack-plugin issues related to @woocommerce/dependency-extraction-webpack-plugin plugin: woocommerce Issues related to the WooCommerce Core plugin. labels Mar 7, 2023
@codecov
Copy link

codecov bot commented Mar 7, 2023

Codecov Report

Merging #37094 (3843cc2) into trunk (6f8f35b) will increase coverage by 0.0%.
The diff coverage is 100.0%.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff            @@
##             trunk   #37094   +/-   ##
========================================
  Coverage     46.7%    46.7%           
- Complexity   17188    17190    +2     
========================================
  Files          429      429           
  Lines        64821    64834   +13     
========================================
+ Hits         30251    30275   +24     
+ Misses       34570    34559   -11     
Impacted Files Coverage Δ
plugins/woocommerce/includes/class-wc-tracker.php 89.8% <100.0%> (+0.1%) ⬆️
.../includes/import/class-wc-product-csv-importer.php 75.1% <100.0%> (+0.2%) ⬆️
...lugins/woocommerce/includes/wc-order-functions.php 76.4% <100.0%> (+0.1%) ⬆️
...lugins/woocommerce/includes/wc-stock-functions.php 78.2% <100.0%> (+0.1%) ⬆️

... and 6 files with indirect coverage changes

@github-actions
Copy link
Contributor

github-actions bot commented Mar 7, 2023

Test Results Summary

Commit SHA: 3843cc2

Test 🧪Passed ✅Failed 🚨Broken 🚧Skipped ⏭️Unknown ❔Total 📊Duration ⏱️
API Tests25900202610m 54s
E2E Tests189006019515m 43s

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.

@mattsherman mattsherman requested a review from a team March 7, 2023 19:53
@mattsherman mattsherman marked this pull request as ready for review March 7, 2023 19:54
@@ -0,0 +1 @@
pacakge-lock=false
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo here!

@@ -27,7 +29,10 @@ export const WC_FOOTER_SLOT_NAME = 'woocommerce_footer_item';
* @param {Array} param0.children - Node children.
* @param {Array} param0.order - Node order.
*/
export const WooFooterItem: React.FC< { order?: number } > & {
export const WooFooterItem: React.FC< {
children?: React.ReactNode;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did children was added here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In React 18, the children prop is no longer implicitly on FC. So, you have to explicitly define it.

https://www.newline.co/@kchan/what-happened-to-the-fc-types-implicit-children-prop-in-typesreact-v18--38576c59

@nathanss
Copy link
Contributor

nathanss commented Mar 8, 2023

I couldn't find any issues with smoke tests! All good. Please ping me again when you fix the conflicts and I'll re-review 😄

@mattsherman
Copy link
Contributor Author

@nathanss Ready for re-review, though be aware that we may go with @woocomerce/admin-header and @woocommerce/admin-footer.

@mattsherman
Copy link
Contributor Author

@samueljseay

Output of pnpm syncpack list-matches:

= Version Group 10 =============================================================
✘ @wordpress/a11y is pinned in this version group at wp-6.0
✘ @wordpress/api-fetch is pinned in this version group at wp-6.0
✘ @wordpress/base-styles is pinned in this version group at wp-6.0
✘ @wordpress/browserslist-config is pinned in this version group at wp-6.0
  ^4.1.1 in devDependencies of packages/js/admin-layout/package.json
✘ @wordpress/components is pinned in this version group at wp-6.0
  ^19.5.0 in dependencies of packages/js/admin-layout/package.json
✘ @wordpress/compose is pinned in this version group at wp-6.0
✘ @wordpress/core-data is pinned in this version group at wp-6.0
✘ @wordpress/data is pinned in this version group at wp-6.0
✘ @wordpress/data-controls is pinned in this version group at wp-6.0
✘ @wordpress/date is pinned in this version group at wp-6.0
✘ @wordpress/deprecated is pinned in this version group at wp-6.0
✘ @wordpress/dom is pinned in this version group at wp-6.0
✘ @wordpress/dom-ready is pinned in this version group at wp-6.0
✘ @wordpress/element is pinned in this version group at wp-6.0
  ^4.1.1 in dependencies of packages/js/admin-layout/package.json
✘ @wordpress/hooks is pinned in this version group at wp-6.0
✘ @wordpress/html-entities is pinned in this version group at wp-6.0
✘ @wordpress/i18n is pinned in this version group at wp-6.0
✘ @wordpress/icons is pinned in this version group at wp-6.0
✘ @wordpress/interface is pinned in this version group at wp-6.0
✘ @wordpress/keyboard-shortcuts is pinned in this version group at wp-6.0
✘ @wordpress/keycodes is pinned in this version group at wp-6.0
✘ @wordpress/media-utils is pinned in this version group at wp-6.0
✘ @wordpress/notices is pinned in this version group at wp-6.0
✘ @wordpress/plugins is pinned in this version group at wp-6.0
✘ @wordpress/primitives is pinned in this version group at wp-6.0
✘ @wordpress/rich-text is pinned in this version group at wp-6.0
✘ @wordpress/url is pinned in this version group at wp-6.0
✘ @wordpress/warning is pinned in this version group at wp-6.0

However, running pnpm syncpack fix-mismatches does not fix these up (it did fix up @types/wordpress__components and postcss-loader in fc1dfee.

I'm not sure what's going on!

@JamieMason
Copy link

However, running pnpm syncpack fix-mismatches does not fix these up (it did fix up @types/wordpress__components and postcss-loader in fc1dfee.

I'm not sure what's going on!

Hey @mattsherman, it might not be this but the long regex in filter could be the cause as that acts as a global filter which overrides everything else in the config – using just the versionGroups and isIgnored might work out as an easier to read approach to controlling which packages you want to run syncpack on and which you don't.

Docs on filter: https://jamiemason.github.io/syncpack/config/filter

@mattsherman mattsherman requested review from nathanss and a team March 10, 2023 13:08
@mattsherman mattsherman merged commit af24637 into trunk Mar 10, 2023
21 checks passed
@mattsherman mattsherman deleted the add/admin-layout-package branch March 10, 2023 14:58
@github-actions github-actions bot added this to the 7.6.0 milestone Mar 10, 2023
@mattsherman
Copy link
Contributor Author

However, running pnpm syncpack fix-mismatches does not fix these up (it did fix up @types/wordpress__components and postcss-loader in fc1dfee.
I'm not sure what's going on!

Hey @mattsherman, it might not be this but the long regex in filter could be the cause as that acts as a global filter which overrides everything else in the config – using just the versionGroups and isIgnored might work out as an easier to read approach to controlling which packages you want to run syncpack on and which you don't.

Docs on filter: https://jamiemason.github.io/syncpack/config/filter

@JamieMason Thanks for the tip using versionGroups and isIgnored (and for syncpack in general!)! I don't think the filter is the issue, as syncpack was correctly noting the mismatches with syncpack list-matches but not fixing them with pnpm syncpack fix-mismatches . Unless the filtering is implemented differently for the listing and the fixing.

@samueljseay That said, given that we have versionGroups, do we really even need the global filter?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package: dependency-extraction-webpack-plugin issues related to @woocommerce/dependency-extraction-webpack-plugin plugin: woocommerce Issues related to the WooCommerce Core plugin.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Create a @woocommerce/admin-layout package
3 participants