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

Remove editor styles from front-facing stylesheets #24439

Merged
merged 3 commits into from
Aug 11, 2020

Conversation

aristath
Copy link
Member

@aristath aristath commented Aug 7, 2020

In order to avoid some duplicate rules between front-facing styles and editor styles, some rules were placed in front-facing stylesheets.
That saves some time for developers, but at the same time is not optimal for visitors, and that has to be more important...
With WP powering 35%+ of the web, even the smallest amount of data we save saves an enormous amount of energy. Shaving off unused front-facing styles, no matter how small that amount is, adds up to huge energy savings and promotes a more sustainable web. It's data that is saved on each page load, on a gigantic scale so every byte has to count.

This PR moves editor styles to editor.scss files. In the case of colors, in order to avoid duplicating a huge chunk of code, these were moved to mixins which then get included where needed.

@ZebulanStanphill ZebulanStanphill added [Feature] Themes Questions or issues with incorporating or styling blocks in a theme. CSS Styling Related to editor and front end styles, CSS-specific issues. labels Aug 7, 2020
@jasmussen
Copy link
Contributor

What a wonderful PR. Thank you.

High level: I agree with the assessment, code looks good, and this all tests well for me. I'm definitely 👍 👍 on this one.

It's also a biggish change. What's the best way of testing that this doesn't regress any color related aspects of themes? Outside of testing a variety of themes and testing foreground and background colors as well as gradients?

@aristath
Copy link
Member Author

It's also a biggish change. What's the best way of testing that this doesn't regress any color related aspects of themes? Outside of testing a variety of themes and testing foreground and background colors as well as gradients?

I tested it with the twentytwenty and another random theme, didn't see any issues (had to disable custom palettes from the theme and enable gradients)

The PR may look a bit biggish, but in essence it just moved around some CSS to be where it's supposed to be. The styles applied are exactly the same as they were.

On a side-by-side comparison I didn't see any differences in the frontend or editor, both for solid colors and gradients 😄

@jasmussen
Copy link
Contributor

I share your assessment that it shouldn't be a massive change. I'm only cautious because I know that there are often ripple effects from the preset colors and gradients. So I hope that Kjell (CC'ed as reviewer) has time to take a quick look, and hopefully we can get this in soon. Thanks again.

Copy link
Contributor

@kjellr kjellr left a comment

Choose a reason for hiding this comment

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

I share your assessment that it shouldn't be a massive change. I'm only cautious because I know that there are often ripple effects from the preset colors and gradients. So I hope that Kjell (CC'ed as reviewer) has time to take a quick look, and hopefully we can get this in soon. Thanks again.

This looks great to me, thanks @aristath!

@kjellr kjellr merged commit 59fa9eb into WordPress:master Aug 11, 2020
@github-actions github-actions bot added this to the Gutenberg 8.8 milestone Aug 11, 2020
@aristath aristath deleted the aristath/no-editor-styles-on-front branch August 11, 2020 13:30
@jasmussen
Copy link
Contributor

🔥🔥🔥

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CSS Styling Related to editor and front end styles, CSS-specific issues. [Feature] Themes Questions or issues with incorporating or styling blocks in a theme.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants