-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
Use inline styles. #4824
Use inline styles. #4824
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? |
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.
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.
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.
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. |
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.
See #4824 (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 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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>
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.
One tiny suggestion more
Co-authored-by: Weston Ruter <westonruter@google.com>
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.
LGTM!
@felixarntz IDE type hinting was removed in 5b8bc3c |
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.
Thanks @spacedmonkey, LGTM!
Committed 54c4de1 |
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.