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

Hydrate product editor settings #37123

Merged
merged 7 commits into from Mar 10, 2023

Conversation

louwie17
Copy link
Contributor

@louwie17 louwie17 commented Mar 8, 2023

All Submissions:

Changes proposed in this Pull Request:

Closes #37120 .

How to test the changes in this Pull Request:

  1. Load this branch and make sure the new-product-management-experience feature is enabled
  2. Go to Products > Add New and make sure the initial block editor still loads correctly.
  3. Open your console and type in productBlockEditorSettings and make sure it is an object contain a bunch of the editor settings.

*Note: I did already add an initial template, but this won't take effect until we integrate the entity store which may be done as part of this PR: #37003

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.

@github-actions github-actions bot added focus: react admin [team:Ghidorah] plugin: woocommerce Issues related to the WooCommerce Core plugin. labels Mar 8, 2023
@louwie17 louwie17 requested a review from joshuatf March 8, 2023 13:53
@github-actions
Copy link
Contributor

github-actions bot commented Mar 8, 2023

Test Results Summary

Commit SHA: 612800b

Test 🧪Passed ✅Failed 🚨Broken 🚧Skipped ⏭️Unknown ❔Total 📊Duration ⏱️
API Tests25900202610m 50s
E2E Tests189006019514m 44s

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
Copy link

codecov bot commented Mar 8, 2023

Codecov Report

Merging #37123 (5bd8518) into trunk (9776cad) will increase coverage by 0.0%.
The diff coverage is n/a.

❗ Current head 5bd8518 differs from pull request most recent head 612800b. Consider uploading reports for the commit 612800b to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff            @@
##             trunk   #37123   +/-   ##
========================================
  Coverage     46.7%    46.7%           
+ Complexity   17191    17190    -1     
========================================
  Files          429      429           
  Lines        64835    64832    -3     
========================================
  Hits         30275    30275           
+ Misses       34560    34557    -3     
Impacted Files Coverage Δ
...ugins/woocommerce/includes/class-wc-post-types.php 2.9% <ø> (ø)

... and 1 file with indirect coverage changes

Copy link
Contributor

@joshuatf joshuatf left a comment

Choose a reason for hiding this comment

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

Nicely done! Just left a couple minor comments.

@@ -14,6 +17,13 @@ const dummyProduct = {
short_description: 'Short product description content',
} as Product;

declare const productBlockEditorSettings: ProductEditorSettings;
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we simplify this naming since we don't use blocks in the other component names?

Suggested change
declare const productBlockEditorSettings: ProductEditorSettings;
declare const productEditorSettings: ProductEditorSettings;

@@ -366,6 +366,14 @@ public static function register_post_types() {
'show_in_nav_menus' => true,
'show_in_rest' => true,
'rest_namespace' => 'wp/v3',
'template' => array(
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice 👍

$editor_settings['templateLock'] = ! empty( $post_type_object->template_lock ) ? $post_type_object->template_lock : false;
}

if ( wp_is_block_theme() && $editor_settings['supportsTemplateMode'] ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

We probably need to check if this property is set first.

Screen Shot 2023-03-08 at 10 17 00 AM

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I actually decided to remove this condition, after re-reading what this logic does it seemed not overly relevant for our use case ( I think ): e8a212f
Given both conditions are to see if we have a block theme and if the theme supports block templates (neither of which we actually care about as the product screen is not theme dependant ).

@louwie17 louwie17 force-pushed the add/37120_hydrate_product_editor_settings branch from f3b4f77 to e8a212f Compare March 9, 2023 08:57
@louwie17 louwie17 requested a review from joshuatf March 9, 2023 09:02
@louwie17
Copy link
Contributor Author

louwie17 commented Mar 9, 2023

@joshuatf this should be good for a re-review.

@louwie17 louwie17 force-pushed the add/37120_hydrate_product_editor_settings branch from e8a212f to 5bd8518 Compare March 9, 2023 19:17
joshuatf
joshuatf previously approved these changes Mar 9, 2023
Copy link
Contributor

@joshuatf joshuatf left a comment

Choose a reason for hiding this comment

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

Thanks for the changes and rebase! Testing well and code looks good.

wp_enqueue_script( $script_handle );
wp_add_inline_script(
$script_handle,
'var productBlockEditorSettings = productBlockEditorSettings || ' . wp_json_encode( $editor_settings ) . ';',
Copy link
Contributor

Choose a reason for hiding this comment

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

We discussed briefly today in our call, but we might update this to blockProductEditorSettings and probably the same for this file name to avoid any ambiguity in naming.

We can discuss over Slack though; no reason to hold up this PR.

Copy link
Contributor

@joelclimbsthings joelclimbsthings left a comment

Choose a reason for hiding this comment

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

Hey @louwie17 , this was already approved and ready to go other than a quick rebase. I went ahead and did so, retested, and I'll likely merge. Hope you don't mind! :shipit:

@joelclimbsthings joelclimbsthings merged commit 329b0cb into trunk Mar 10, 2023
27 checks passed
@joelclimbsthings joelclimbsthings deleted the add/37120_hydrate_product_editor_settings branch March 10, 2023 20:21
@github-actions github-actions bot added this to the 7.6.0 milestone Mar 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
plugin: woocommerce Issues related to the WooCommerce Core plugin.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Hydrate product block editor settings
3 participants