-
Notifications
You must be signed in to change notification settings - Fork 10.7k
Fix editor header hidden in Firefox #38247
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
Conversation
|
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: |
Test Results SummaryCommit SHA: 6691e05
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
left a comment
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.
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.
joshuatf
left a comment
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 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, |
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'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.
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'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.
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.
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.
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'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.
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. |
6cbd291 to
cefde8d
Compare
|
@joshuatf this is ready for re-review |
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## trunk #38247 +/- ##
========================================
Coverage 51.6% 51.6%
Complexity 17444 17444
========================================
Files 440 440
Lines 80297 80297
========================================
Hits 41401 41401
Misses 38896 38896
|
joshuatf
left a comment
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.
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.
cefde8d to
6691e05
Compare
joshuatf
left a comment
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.
Still LGTM! 🚢
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:
product-block-editorfeature flag