Skip to content

Conversation

@louwie17
Copy link
Contributor

Submission Review Guidelines:

Changes proposed in this Pull Request:

This removes the use of the has() css selector (not supported by Firefox), and replaces it with some logic to add a body class when the product block editor is rendered. Then hiding the header if that class is in use.

Closes #38242 .

How to test the changes in this Pull Request:

Using the WooCommerce Testing Instructions Guide, include your detailed testing instructions:

  1. Load this branch and enable the product-block-editor feature flag
  2. Load this in Firefox and go to Products > Add New
  3. The header should load correctly with the product title and save buttons

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

github-actions bot commented May 11, 2023

Hi @joshuatf,

Apart from reviewing the code changes, please make sure to review the testing instructions as well.

You can follow this guide to find out what good testing instructions should look like:
https://github.com/woocommerce/woocommerce/wiki/Writing-high-quality-testing-instructions

@github-actions
Copy link
Contributor

github-actions bot commented May 11, 2023

Test Results Summary

Commit SHA: 6691e05

Test 🧪Passed ✅Failed 🚨Broken 🚧Skipped ⏭️Unknown ❔Total 📊Duration ⏱️
API Tests26700202691m 9s
E2E Tests1890010019914m 20s

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.

mdperez86
mdperez86 previously approved these changes May 12, 2023
Copy link
Contributor

@mdperez86 mdperez86 left a comment

Choose a reason for hiding this comment

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

Testing browsers

  • Firefox 113.0.1 (64-bit)
  • Opera 98.0.4759.39 (arm64)
  • Edge 115.0.1843.0 (Official build) Dev (arm64)
  • Chrome 112.0.5615.137 (Official Build) (arm64)
  • Safari 16.4 (18615.1.26.110.1)
  • Chromium 107.0.5298.0 (Developer Build) (arm64)

LGTM! Nice job @louwie17.

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.

I left some implications of going this route. I'm leaning towards a CSS solution, but let me know if you disagree or see downsides to that.

I've introduced body classes in #38281 that could help with this and happy to include the fix there or we can rebase and update here if we decide CSS is the right path forward.

navArgs: {
id: 'woocommerce-add-product',
},
showHeader: false,
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure introducing a new property here is the best path. If we allow hiding the header here, will we also introduce various other properties for hiding activity panels, notices, etc on each page?

It seems like CSS may be an easier solution and more accommodating for more cases.

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'm not sure introducing a new property here is the best path. If we allow hiding the header here, will we also introduce various other properties for hiding activity panels, notices, etc on each page?

I would say 'yes' as demand for this emerges.
CSS seems fine to me also, and was originally my first solution, but I would prefer to do these things within the code if possible. Unless there is a strong reason for still rendering the Header in the background, but this may cause unexpected side affects as well, especially given it can be slot filled.

But given you are introducing the css body classes in your PR, I can forego going with that approach.

Copy link
Contributor

Choose a reason for hiding this comment

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

but I would prefer to do these things within the code if possible

I would also be in favor of handling these in the code where possible, but I'm hesitant to introduce knowledge around the various frontend components into the router/controller API as this could grow indefinitely.

Perhaps a more composable solution would be something good to add in the future, but I think this might be a lot for this PR to tackle.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm second guessing myself here just out of fear of possible side effects. Maybe for now we go back to your solution and introduce a property under pages that controls various aspects of the layout:

pages.push( {
	container: ProductPage,
	path: '/add-product',
	layout: {
		header: false,
	},
} );

That doesn't do a great job of mitigating growth of layout-specific properties in the controller, but at least puts these items under a more identifiable category.

@louwie17
Copy link
Contributor Author

I left some implications of going this route. I'm leaning towards a CSS solution, but let me know if you disagree or see downsides to that.

I've introduced body classes in #38281 that could help with this and happy to include the fix there or we can rebase and update here if we decide CSS is the right path forward.

Gotcha, I am ok with using CSS as well, that was actually my original approach: 0722030 where the editor adds a class to the body tag.
I can revert to that change if that looks good to you?

@joshuatf
Copy link
Contributor

joshuatf commented May 15, 2023

@louwie17 Left a comment here - I think CSS is the best short-term solution until we refactor our layout components to be more reusable.

Left another comment. I think your concerns are valid; just want to make sure these controllers don't become unmanageable over time as the application grows.

@louwie17 louwie17 force-pushed the fix/38242_editor_hidden_in_firefox branch 2 times, most recently from 6cbd291 to cefde8d Compare May 17, 2023 10:53
@louwie17 louwie17 requested a review from joshuatf May 17, 2023 10:53
@louwie17
Copy link
Contributor Author

@joshuatf this is ready for re-review

@codecov
Copy link

codecov bot commented May 17, 2023

Codecov Report

Merging #38247 (cefde8d) into trunk (e077c4c) will not change coverage.
The diff coverage is 66.7%.

❗ Current head cefde8d differs from pull request most recent head 6691e05. Consider uploading reports for the commit 6691e05 to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff            @@
##             trunk   #38247   +/-   ##
========================================
  Coverage     51.6%    51.6%           
  Complexity   17444    17444           
========================================
  Files          440      440           
  Lines        80297    80297           
========================================
  Hits         41401    41401           
  Misses       38896    38896           
Impacted Files Coverage Δ
plugins/woocommerce/includes/class-woocommerce.php 20.0% <0.0%> (ø)
...cludes/data-stores/class-wc-webhook-data-store.php 78.5% <80.0%> (ø)

joshuatf
joshuatf previously approved these changes May 17, 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.

Testing well! Thanks for putting up with my back and forth on this.

There are a couple of what look to be unrelated failing tests. I'll try to restart them to get them passing, but feel free to rebase and re-ping me if needed.

@louwie17 louwie17 force-pushed the fix/38242_editor_hidden_in_firefox branch from cefde8d to 6691e05 Compare May 18, 2023 08:39
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.

Still LGTM! 🚢

@louwie17 louwie17 merged commit 2843fbb into trunk May 18, 2023
@louwie17 louwie17 deleted the fix/38242_editor_hidden_in_firefox branch May 18, 2023 16:00
@github-actions github-actions bot added this to the 7.8.0 milestone May 18, 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.

Product Block Editor: Editor Header is hidden on Firefox

4 participants