Conversation
Co-authored-by: Weston Ruter <westonruter@google.com>
|
@westonruter I have reverted a lot of the changes and found work arounds for other changes. Can you take a look again? |
felixarntz
left a comment
There was a problem hiding this comment.
Some of the changes in the PR look good to me, but others introduce breaking changes we can and should avoid. I left more concrete feedback below.
felixarntz
left a comment
There was a problem hiding this comment.
Thanks for the updates @spacedmonkey, LGTM!
|
Awaiting @westonruter review here and I will commit. |
Please do not commit this unless it is proven useful. As it stands now this partial refactoring doesn't seem like a good idea. See: https://core.trac.wordpress.org/ticket/58775#comment:16. Also there aren't any use cases for it. Why would that be in core? Also please keep in mind that CSS does work quite differently than JS and PHP. The "Cascading" part in the name means styles are "cascaded", i.e. depend on the underlying styles. Because of this removing or tweaking the default WP styles is generally a bad idea and can be considered backwards compatibility breach. However enabling the removal or tweaking of core's CSS seems to be the only benefit of this partial refactoring. So thinking that this should not be added to core. |
felixarntz
left a comment
There was a problem hiding this comment.
I have one additional point of feedback here which I had missed before.
src/wp-includes/admin-bar.php
Outdated
| return; | ||
| } | ||
| remove_action( $action, 'wp_admin_bar_header' ); | ||
| wp_add_inline_style( 'admin-bar', /* language=CSS */ '@media print { #wpadminbar { display:none; } }' ); |
There was a problem hiding this comment.
See #4773 (comment), I don't think we should have IDE specific annotations in core, unless they are a standard pattern supported by various IDEs.
That is not a benefit of this PR. You can already do that today by unhooking the functions that print it.
This is in fact a good reason for this change: The PR ensures those core styles make use of the |
Co-authored-by: Weston Ruter <westonruter@google.com>
Co-authored-by: Weston Ruter <westonruter@google.com>
Co-authored-by: Weston Ruter <westonruter@google.com>
|
@felixarntz IDE type hinting was removed in 5b8bc3c |
felixarntz
left a comment
There was a problem hiding this comment.
Thanks @spacedmonkey, LGTM!
|
Committed 54c4de1 |
|
As far as I can tell, I think this PR has introduced a regression for editor styles in the block editor.
When looking at the markup for the post editor, there appears to now be a deprecation notice injected into 
|
Trac ticket: https://core.trac.wordpress.org/ticket/58775
This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.