Add postcss-modules to support local selectors #61940
Conversation
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
Size Change: +22 B (0%) Total Size: 1.75 MB
ℹ️ View Unchanged
|
|
This seems like a sensible solution to me 👍
I've added a commit to resolve that - seems like we need to ignore it specifically, because the Not sure about the VSCode error at a first glance 🤔 |
Turns out to be a problem with the VSCode's CSS language server and its CSS parser. Reported it here: microsoft/vscode-css-languageservice#393 So, the |
| require( 'postcss-modules' )( { | ||
| scopeBehaviour: 'global', | ||
| getJSON: () => {}, | ||
| } ), |
There was a problem hiding this comment.
I'm not entirely sure if it's appropriate/necessary to add this to @wordpress/postcss-plugins-preset, rather than just for the Gutenberg project. I feel like most WP extenders would not need this, or will have their own setup if they do. It's also non-obvious that @wordpress/postcss-plugins-preset would support CSS Modules by default. Thoughts?
There is also a possibility that we will be using CSS Modules for wp-components, and in that case we will need to add additional config (to un-noop the getJSON, at the very least) for the Gutenberg project anyway. Not super relevant to the decision we make here, but just adding as an FYI.
There was a problem hiding this comment.
I agree that we can start with enabling postcss-modules only for the Components package 👍
There was a problem hiding this comment.
What's the appropriate place to add this config only to Gutenberg?
There was a problem hiding this comment.
Gutenberg's webpack config in tools/webpack perhaps?
There was a problem hiding this comment.
I think all you need is to change the config passed to postcss() here:
https://github.com/WordPress/gutenberg/blob/add/postcss-modules/bin/packages/build-worker.js#L126
It should configure postcss with both the preset and the additional plugin.
There was a problem hiding this comment.
Great thank you, now I'm gonna try to test it in my branch to see if it has the desired effect. If you have suggestions on how to find the output that'd be a helpful shortcut :)
There was a problem hiding this comment.
Run npm run build:packages. That should build all packages, including styles.
Then look at packages/edit-site/build-style/style.css. That's the output of the build, all edit-site styles processed and concatenated together.
There is no bundler in this build pipeline, it's just the bin/packages/build.js script spawning a farm of bin/packages/build-worker.js scripts.
There was a problem hiding this comment.
Love it! Simple and effective.
I do wish that we had a way to implement this without the need for :local at all. What I mean by that is something like having everything be global by default, and then have specific things like animations be local by default. That way, the source would just look like normal CSS, and we wouldn't need the :local pseudoclass in the declaration nor in the usage.
For now I think this is perfectly fine, though I might take a stab at implementing a simple PostCSS plugin that does what I described at some point.
This is what the PR mostly does: all styles are still global, and only these explicitly marked as Do you mean a variation of this that just mangles all keyframes names automatically, without tagging them? For that we could indeed implement our own PostCSS plugin. Modeled after https://github.com/css-modules/postcss-modules-scope/blob/master/src/index.js For our purposes we could heavily simplify this. Just walk the AST, find all |
|
@jsnajdr yes, that is what I propose, have those be local by default (and then possibly allow |
|
@jsnajdr I found this plugin that does what we need, more or less. I do not love that the prefix is fixed, since that could potentially result in someone re-using a local animation. It also does not have a way to opt out of it. It could be a better starting point for a fork though. Let me do a quick attempt and see what I can come up with. Edit: forgot to link the plugin lol - https://github.com/VitaliyR/postcss-prefix-keyframe |
|
Yes,
Yes, I think it would be a much better solution that this PR currently is 👍 Go for it! |
|
WIP plugin here, already does what I proposed so far: https://github.com/DaniGuardiola/postcss-local-keyframes If there are no objections or further improvements, I will publish to npm soon and then create a PR here that uses it. |
There was a problem hiding this comment.
I prefer making variables local by default and global when needed, as it's safer and provides better DevEX. However, considering the current feature requirements, I'd say shipping this now is fine while we evaluate @DaniGuardiola's plugin, as migrating to a new approach would be simple 👍🏻
|
@fullofcaffeine I fully understand your reasoning, though to be clear, these "local" and "global" concepts are fully dependent on a commitment to using CSS modules, which in my understanding is not what we're doing here. In fact, I personally don't think that's the best idea, and have some ideas to handle style scoping in the future that I will be proposing and experimenting with (in a nutshell, use the native CSS feature for scoping, This PR is (again, in my understanding) about |
That means full commitment to CSS modules and major rewrite of all CSS files. This PR is much less ambitious: it's using CSS modules only to mark a very small numbers of selectors as |
Understood, thanks for elaborating! |
|
Closing in favor of #62476. It was a nice experiment 🙂 |
Adds support for marking selectors or keyframes as
:local, and preventing them from conflicting with other global CSS definitions.See how I'm using it to locally scope the
slide-from-rightsidebar animations in Site Editor. The animation names no longer pollute the global namespace. They are mangled into names like_slide-from-left_1uetc_2516.There are still open problems. Stylelint doesn't understand the
:localsyntax and complains:Also, VSCode complains about the syntax:
Here I'm not even sure where the error comes from. Is it a built-in VSCode SCSS syntax checker? Or is it a lint plugin that the Gutenberg repo can configure?
How to test:
Go to Site Editor and verify that the sidebar animations (sliding as you navigate back and forth) still work: