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

Fix Cover CSS specificity #12688

Closed
wants to merge 5 commits into from

Conversation

m-e-h
Copy link
Member

@m-e-h m-e-h commented Dec 7, 2018

Description

Reduces specificity in core Cover styles to ease clashes with theme styles.
This has been explained and discussed in further detail here #11779 (comment)

The cover text color was added to the parent .wp-block-cover element so that text.wp-block-cover-text and links.wp-block-cover-text a could inherit the color. This drys up the code and allows for single class selector overrides.

I've also removed the references and styles for .wp-block-cover-image. I couldn't find anything mentioning a planned removal date and assumed now would be OK to do so..?

I'm also not sure what the h2 styles are referencing. Is an h2 something I'm missing within the Cover Block? I'd like to remove if not.

How has this been tested?

visual tests were done in browser.

Types of changes

Bug fix (non-breaking change which fixes an issue)

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.

Reduces specificity in core Cover styles to ease clashes with theme styles.
This has been explained and discussed in further detail here WordPress#11779 (comment)

The cover text color was added to the parent `.wp-block-cover` element so that text`.wp-block-cover-text` and links`.wp-block-cover-text a` could `inherit` the color. This drys up the code and allows for single class selector overrides.

I've also removed the references and styles for `.wp-block-cover-image`. I couldn't find anything mentioning a planned removal date and assumed now would be OK to do so..?

I'm also not sure what the `h2` styles are referencing. Is an `h2` something I'm missing within the Cover Block? I'd like to remove if not.
@gziolo gziolo added the [Block] Cover Affects the Cover Block - used to display content laid over a background image label Dec 14, 2018
@gziolo gziolo added Needs Design Feedback Needs general design feedback. [Type] Enhancement A suggestion for improvement. labels Dec 14, 2018
@jasmussen
Copy link
Contributor

Thank you for your pull requests!

Before:

screenshot 2018-12-17 at 09 30 36

After:

screenshot 2018-12-17 at 09 30 12

I deeply appreciate the desire to improve the specificity of CSS to simplifiy things for developers. However in this case, I feel like the visual change is a little drastic.

One of the things we need to explore for phase 2, is to enable nested blocks inside the Cover block. This would allow you to insert a H2 if you liked, in which case the changes made in this PR would make total sense.

However in absence of that, I'm not sure we should make the change from H2 to P quite at this point. The Cover block is intended to be bold, and this definitely makes it less so. There are even a number of other checks in place that assume the Cover block is a H2 and therefore includes it as a transform from the Header block, and counts it in the Table of Contents. In that way it's sort of a breaking change.

Is there a way we can turn this back into a H2 for now, yet have some of the benefits of the CSS specificity improvements made?

@@ -1,6 +1,6 @@
.wp-block-cover-image,
Copy link
Contributor

Choose a reason for hiding this comment

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

The block used to be called "Cover Image". Do we need a deprecation handler for people who inserted the block when it was called that, and who rely in this CSS?

Keep in mind someone might have installed the plugin midways, and created a bunch of reusable blocks of this. These might break?

@m-e-h
Copy link
Member Author

m-e-h commented Dec 17, 2018

I think the visual change is more a symptom of this issue #11200 (comment) and the tinymce styles in general.

.editor-styles-wrapper .mce-content-body {
    line-height: 1.8;
}

.editor-styles-wrapper p {
    font-size: 16px;
    line-height: 1.8;
}

I don't see any visual change at all on the front-end.

My idea with PRs like this one is to get the CSS to an extendable and easily theme-able state.
The need to adapt our block CSS to accommodate an unnecessary style like .editor-styles-wrapper p will only lead to technical debt for this project and for the themes and plugins that are now required to accommodate our accommodation of the unnecessary style .editor-styles-wrapper p.

That chain of events might look something like this:

Inherited style
.editor-styles-wrapper p

Cover style.scss
.wp-block-cover .wp-block-cover-text

Theme style.css
.entry .wp-block-cover .wp-block-cover-text

Child Theme style.css
#content .wp-block-cover .wp-block-cover-text 😷

As for the old Cover Image block, not sure what the protocol is for that.
Personally, I wouldn't deprecate it. I'd just remove it.
Anyone relying on "pre-RC" code probably needs a little push to go ahead and get caught up.
I guess it probably should've been removed in the RC though...

@jasmussen
Copy link
Contributor

I feircely agree with your end goal of simpler theming, absolutely. But I need to make sure I understand this correctly:

I think the visual change is more a symptom of this issue #11200 (comment) and the tinymce styles in general.

Turns out you're absolutely right. Cover used feature a H2 headline inside, but this appears to have changed to a P. That also explains why the h2 CSS is still there, that's clearly vestigial at this point and should be removed. This also means much of what I said in my last comment is moot :D sorry about that, thanks for the patience.

However given that the Cover Image is still intended to be a bold one, even though it's now a P, I feel it should still visually be the same as it was, i.e. as big as the H2.

It's a balance to tread between what styles a block should be born with, and what styles should be opt in. Since Cover is entirely new, and no existing WordPress themes have any awareness of it, the font size should be part of style.scss, not theme.scss, because that's how the block was designed to look.

@m-e-h
Copy link
Member Author

m-e-h commented Dec 17, 2018

Definitely agree the cover text should be bold. And it still is when looking at the front-end with this PR. But in the editor it's overridden by .editor-styles-wrapper p {

I guess my bigger question and maybe a topic for a different issue or PR:
Why does 99% of this file exist? https://github.com/WordPress/gutenberg/blob/master/packages/editor/src/editor-styles.scss
Do the elements we're styling in that file not have a more specific class we can use?
Or is there a more specific part of the editor we could scope them too so that it doesn't interfere with the actual block styles.

HTML tags are at the core of front-end development and as semantic, open standards, they should be freely available and encouraged for use.

But what WP Core is doing by strengthening the specificity here and with the editor styles at large, is saying:

All HTML Elements are reserved for WP editor use only.

They're more or less CSS "reserved words" in the editor. It feels very wrong to me.

@jasmussen
Copy link
Contributor

Definitely agree the cover text should be bold. And it still is when looking at the front-end with this PR. But in the editor it's overridden by .editor-styles-wrapper p {

Perhaps then we can add the font size to the editor.scss so the visual is the same?

I guess my bigger question and maybe a topic for a different issue or PR: Why does 99% of this file exist? https://github.com/WordPress/gutenberg/blob/master/packages/editor/src/editor-styles.scss Do the elements we're styling in that file not have a more specific class we can use? Or is there a more specific part of the editor we could scope them too so that it doesn't interfere with the actual block styles.

That's the vanilla stylesheet, it provides a baseline look and feel for any theme that does not provide its own editor style.

I get your meta question, which is: can we not load that file if a theme provides its own editor style? And the answer is: I'd like that too, but I don't think we can ditch it quite yet.

The biggest reason is that the editing canvas is not inside an iframe, nor is it in the Shadow DOM. Therefore it inherits every CSS style from wp-admin itself. If we did not provide those styles, the editing canvas would more or less look like any other settings page in wp-admin, inheriting the same styles, fonts, margins, paddings, colors.

HTML tags are at the core of front-end development and as semantic, open standards, they should be freely available and encouraged for use.
But what WP Core is doing by strengthening the specificity here and with the editor styles at large, is saying: All HTML Elements are reserved for WP editor use only.

Forgive me if I'm restating something obvious and known to you, but it's pertinent to the discussion here. When you load an editor style into Gutenberg using add_editor_style, the contents of that file are rewritten.

If you write p { color: red; }, it is rewritten as .editor-styles-wrapper p { color: red; }. If you write body { background: green; } it is rewritten as .editor-styles-wrapper { background: green; }.

Yes, this both rewrites your editor style, and it strengthens the specificity, and the purpose is to override the CSS bleed we receive from wp-admin. It is not meant to be nefarious or a way to reserve keywords, the actual goal is to allow you to continue to write your editor styles as you did in the past — write pure CSS as you normally would.

The long term utopian goal, even, is to load the theme stylesheet itself into the editor, and not require a separate editor style.

But as you imply by creating this pull request in the first place: this is still far from perfect, and we have a long way to go. One personal frustration I have is that I can't do body { color: red; }, I have to specify the p instead.

The utopian dream is achievable with incoming browser technologies.

  • Using Shadow DOM we can potentially protect the entire editing canvas from inheriting CSS from the admin.
  • By improving how the admin CSS is written, we can further scope and refine things.
  • By using display: contents; for blocks in the editor, we can level the playing field so the markup for the purposes of CSS styling is virtually identical between the frontend and backend.
  • By using element queries instead of media queries, the responsiveness can be accurate in both instances.
  • Using CSS exclusions, we can use a CSS grid layout that accommodates floats without using crazy hacks.

But all of those CSS techniques are still not widely enough supported, and until we drop support for IE11, all bets are off.

So why isn't the editing canvas in an iframe? Aside from the technical and performance challenge (latency in and out of the canvas), things like the block preview in the block library would either also require small iframes, or just not work. This preview is especially important as reusable blocks and templates become more widely used. For example I'd love to see a prompt to let you pick the page template when you click "Add New Page" in phase 2 Gutenberg, and let you see small thumbnails of each template.

SO, in the mean time we are left with improving our CSS, reducing specificity where we can, and adding new rewrite rules to the editor style engine. Is that more or less accurate, @youknowriad?

Would it help if there was an option the knowing themer like yourself to say "I know what I'm doing, don't load the vanilla stylesheet at all plz"?

@m-e-h
Copy link
Member Author

m-e-h commented Dec 18, 2018

Always impressed with your thoughtful and courteous replies @jasmussen. Thanks so much for taking the time and being patient with my concerns!

I'll revise this pull so that it works with the current state of things.

Unfortunately I'm still not satisfied with the current state of things 😉
But I'll open discussion for that in a different issue or PR and leave this PR to the Cover Block.

@jasmussen
Copy link
Contributor

Your own contributions and courteousness deserves only the same in kind.

Also to be clear I'm not asking you to be satisfied with the current state of things, neither am I. Just that we have to improve the state of things on a larger scale, step by step. Your tickets on specificity all help.

@gziolo gziolo added the [Status] Stale Gives the original author opportunity to update before closing. Can be reopened as needed. label Feb 6, 2019
@gziolo
Copy link
Member

gziolo commented Feb 6, 2019

@m-e-h, as I'm triaging older PRs I noticed that this one hasn't been updated yet. I'm marking it as Stale to indicate that it needs to be refreshed and therefore doesn't have to be reviewed at the moment. I'm happy to remove this label as soon as comments are addressed.

@m-e-h
Copy link
Member Author

m-e-h commented Feb 8, 2019

This PR should be good now that 3148d8c has been merged. I'll test it out and post the results.

@gziolo gziolo removed the [Status] Stale Gives the original author opportunity to update before closing. Can be reopened as needed. label Feb 8, 2019
@gziolo gziolo added this to the 5.2 (Gutenberg) milestone Feb 8, 2019
@youknowriad youknowriad removed this from the 5.3 (Gutenberg) milestone Mar 18, 2019
@ZebulanStanphill
Copy link
Member

Is this PR still relevant/necessary?

@youknowriad
Copy link
Contributor

Hi There! I've triaging PRs today and it seems this one is stale. I'm going to close now but please consider reopening if you have some time to dedicate to it. Thanks for your efforts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Cover Affects the Cover Block - used to display content laid over a background image Needs Design Feedback Needs general design feedback. [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants