mrwweb / useful-block-styles

Simple block styles you frequently need for the WordPress Block Editor
GNU General Public License v3.0
4 stars 1 forks source link

Removing !importants; using dep in enqueue, instead. #5

Closed rtvenge closed 3 years ago

rtvenge commented 3 years ago

@mrwweb would this work instead for the styles? I would see that the dev might want to slightly override your styles on the front end, and battling !important's would make that difficult. I think using the dep in the enqueue will accomplish the same thing, right?

mrwweb commented 3 years ago

@rtvenge Very excited to see you in these parts! I think the !importants are still necessary, unfortunately, but it's worth having to justify them!

This post summarizes the thinking.

The key reason is that these styles need to work with any theme and likely should override selectors of higher importance.

To use the no-bullets example, here's the CSS from the plugin without !important:

.is-style-useful-no-markers,
.is-style-useful-no-markers li {
    padding-left: 0;
    list-style: none;
}

That would "lose" out to even a very responsible theme selector like this, regardless of the source order of stylesheets:

.entry-content ul li {
    list-style: square;
}

So my thinking is that:

  1. Use !important to always override default theme styles because block styles are essentially utility classes that you always do exactly what you want them to when selected.
  2. Make the plugin selectors as low-specificity as possible so that overriding them is still easy, even if !important is required.

What do you think?

rtvenge commented 3 years ago

@mrwweb that's a very good point.

I think if we are applying styles that we are 95% sure they wouldn't need to be overridden, then we are good. And you get that protection you mentioned above from resets and other theme styles.

I also like the idea that, worst case, the theme author can use a more specific selector with an !important in that 5% chance they need to override.

Some thoughts for moving forward:

  1. Any harm in keeping the dep in the enqueue seeing as these styles do technically depend on the base block styles?
  2. Could we add a comment to the top of the css files explaining the reasoning around !important so others can see that right away? Maybe just a link to this PR? 😄
mrwweb commented 3 years ago

This discussion is so great and why I wanted eyes on the plugin!

  1. The dependency check seems like a good idea, but I am unsure of one thing. Is that the generic stylesheet block basic styles or the opt-in block stylesheet? If the latter, I don't think we'd want that as a dependency. I've started building my custom themes without that second one. Do you know or do we need to do research.
  2. We should definitely document that in the stylesheet and probably plugin readme too.
rtvenge commented 3 years ago

@mrwweb

  1. I looked into it a little more and it appears that this is the base stylesheet (See here and here).
  2. Great!

I updated the code accordingly.

I also just noticed you use the same technique I do for caching enqueues. 😄

mrwweb commented 3 years ago

Thanks for doing that research, @rtvenge. This looks great (and I dealt with the merge conflict 😃).

I love busting cache this way! I think I first saw it on wp-rig well after you were doing it. FYI: Your code snippets have an alignment bug on your blog. Hopefully that's not a problem caused by wp-block-library 🤣