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

Don't enqueue CSS when post type is not Gutenberg enabled #11673

Closed
jdevalk opened this issue Nov 9, 2018 · 5 comments
Closed

Don't enqueue CSS when post type is not Gutenberg enabled #11673

jdevalk opened this issue Nov 9, 2018 · 5 comments
Labels
[Type] Code Quality Issues or PRs that relate to code quality [Type] Enhancement A suggestion for improvement.

Comments

@jdevalk
Copy link
Contributor

jdevalk commented Nov 9, 2018

Describe the bug
When a post type is not using Gutenberg yet, for instance because the site uses the Gutenberg Ramp plugin, we shouldn't enqueue block-library/style.css and block-library/theme.css on the frontend for that post type.

To Reproduce
Steps to reproduce the behavior:

  1. Go to https://joost.blog/about/, this is a page. On this site, the classic editor is used for this post type.
  2. View source, see that both of these CSS files are embedded anyway.
@Clorith
Copy link
Member

Clorith commented Nov 9, 2018

What then if I have a non-Gutenberg post type that in turn pulls in content from other posts/pages that do Gutenberg (or even sidebars that somehow rely on it)?

I don't think there's much harm in blanket including the Gutenberg styles if Gutenberg is enabled, to avoid any such scenarios (with the exception where themes have dequeued them on their own, in which case they are responsible for things being proper).

Yes, it adds 1 or 2 resources to load, but hopefully our http/2 focus can be re-ignited post 5.0 and that will then just be a temporary tradeoff until those capable have it in their hands. (I don't know how we're looking on adoption of webservers that support it of course, so I can only speak in general terms).

@jdevalk
Copy link
Contributor Author

jdevalk commented Nov 9, 2018

Well, in the current state of those files, they can affect the look of a page without wanting to do so. The CSS isn't all namespaced, for instance, this:

@keyframes fade-in{
    0%{
        opacity:0
    }
    to{
        opacity:1
    }
}

That could very easily conflict with existing CSS. If we want to allow people to "Ramp up" to Gutenberg using Gutenberg ramp, we should make sure we don't break their site design for them.

@Clorith
Copy link
Member

Clorith commented Nov 9, 2018

That's a very valid point, perhaps we should be a bit more mindful here and actually have some form of wrapper class or similar on our styles.. At least this seems like the best approach in my opinion to not unexpectedly affect non-related elements, and it is what we tell themes and plugins to do, so abiding by it our selves as well makes perfect sense.

@jdevalk
Copy link
Contributor Author

jdevalk commented Nov 9, 2018

To be fair, as I mentioned in #11668 I don't think these styles should exist in that file. There's more there that isn't structural but rather meant for the editor. Everything else is namespaced with wp- but IMHO that's not specific enough.

@youknowriad
Copy link
Contributor

(Deleted my comment as published on the wrong issue)

@designsimply designsimply added [Type] Enhancement A suggestion for improvement. [Type] Code Quality Issues or PRs that relate to code quality labels Nov 11, 2018
jasmussen pushed a commit that referenced this issue Nov 12, 2018
This PR maybe fixes #11673, maybe fixes #11668. At least, this is the intention. It does two things:

- It changes the naming scheme for all animations. Instead of having a mix of underscores and dashes for word separation, it uses BEM (to the best of my ability) to add consistency and a new naming convention for all animations.
- It adds a prefix to all animations, indicating they are for the editor. Though perhaps we should have a more _admin_ specific prefix given these might one day be used outside the admin?
- It moves all keyframe animations out of the mixins file, and into the edit-post style. This is because keyframe definitions don't work well with mixins and are just duplicated.

This PR has been tested for all the animations I could find that were using the ones defined. But please give me a sanity check.

By the way I noticed the following two animations do not appear to be used:

- editor-animation__animate-fade
- editor-animation__move-background

Should we remove those? Or can anyone recall where they were intended to be used?
jasmussen pushed a commit that referenced this issue Nov 13, 2018
This PR maybe fixes #11673, maybe fixes #11668. At least, this is the intention. It does two things:

- It changes the naming scheme for all animations. Instead of having a mix of underscores and dashes for word separation, it uses BEM (to the best of my ability) to add consistency and a new naming convention for all animations.
- It adds a prefix to all animations, indicating they are for the editor. Though perhaps we should have a more _admin_ specific prefix given these might one day be used outside the admin?
- It moves all keyframe animations out of the mixins file, and into the edit-post style. This is because keyframe definitions don't work well with mixins and are just duplicated.

This PR has been tested for all the animations I could find that were using the ones defined. But please give me a sanity check.

By the way I noticed the following two animations do not appear to be used:

- editor-animation__animate-fade
- editor-animation__move-background

Should we remove those? Or can anyone recall where they were intended to be used?
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Code Quality Issues or PRs that relate to code quality [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

No branches or pull requests

4 participants