Components: Start using CSS Modules #103004
Conversation
mirka
added
Components
[Type] Tooling
|
Here is how your PR affects size of JS and CSS bundles shipped to the user's browser: App Entrypoints (~78 bytes added 📈 [gzipped]) Common code that is always downloaded and parsed every time the app is loaded, no matter which route is used. Sections (~440 bytes added 📈 [gzipped]) Sections contain code specific for a given set of routes. Is downloaded and parsed only when a particular route is navigated to. Async-loaded Components (~374 bytes added 📈 [gzipped]) React components that are loaded lazily, when a certain part of UI is displayed for the first time. Legend What is parsed and gzip size?Parsed Size: Uncompressed size of the JS and CSS files. This much code needs to be parsed and stored in memory. Generated by performance advisor bot at iscalypsofastyet.com. |
| @@ -1,4 +1,4 @@ | |||
| .wordpress-logo { | |||
| .wordpressLogo { | |||
There was a problem hiding this comment.
Kebab casing also works, but camel case is the official recommendation.
There was a problem hiding this comment.
Kebab casing also works, but camel case is the official recommendation.
It's an interesting topic. It definitely feels more natural in JavaScript to reference with a camelCase property, and ultimately the class names are clobbered before they get to the client so it doesn't matter there.
That said, with all the inherited experience of BEM or general kebab-case styling in CSS, I wonder if this will be an issue for people, especially in transitioning between files with different conventions. Are there ways we can make this easier, or enforce some consistency within respective file types? Thinking documentation or tooling like Stylelint rules.
I also personally don't have a huge issue with referencing the kebab-case properties to keep consistency, even if it's a little less ergonomic.
There was a problem hiding this comment.
No strong opinion on my end. The ergonomics/safety characteristics aren't that different once you're using an IDE extension or the ts plugin.
One argument in favor of camel case is that it'll be more obvious that the file is a module, not a horrible BEM file 😂
There was a problem hiding this comment.
I have a pretty strong opinion to keep using kebab case, at least until it's the official recommendation of the WordPress CSS Coding Standards
| @@ -4,3 +4,13 @@ declare module '*.svg' { | |||
| } | |||
|
|
|||
| declare const __i18n_text_domain__: string; | |||
|
|
|||
| declare module '*.module.css' { | |||
There was a problem hiding this comment.
Keeping this to only @automattic/components for now, but I think we'll also want it in at least client/ as well.
matticbot
added
the
[Status] Needs Review
| @@ -1,4 +1,4 @@ | |||
| .wordpress-logo { | |||
There was a problem hiding this comment.
You may have noticed that there's a redundant copy of this component in calypso/components 😇 #103012
There was a problem hiding this comment.
Let's kill it with fire! A migration is a matter of a single Cursor prompt these days. The v2 hosting dashboard eslint rules might be an exception that will need a manual hand.
There was a problem hiding this comment.
I'd be personally curious for feedback from others on kebab-case vs. camelCase, but otherwise LGTM.
I'm not entirely familiar how we expect this to be integrated into build pipelines, so a few related questions for my own understanding:
- Does this impose additional requirements on downstream consumers of
@automattic/componentsto be able to support CSS modules? Do we need to document that? - Maybe dependent on project & build tool, but where do these styles end up? I see in the Storybook preview they're injected through JavaScript, but would we want/expect them to be loaded as stylesheets in production environments?
| // and should be normalized to be merged instead. Ideally, we also remove the `wordpress-logo` | ||
| // class because it clashes very easily. |
There was a problem hiding this comment.
Ideally, we also remove the
wordpress-logoclass because it clashes very easily.
With the eventual removal of wordpress-logo, what's the desired end-state where a component is currently overriding styles of .wordpress-logo ? Would that component be expected to pass its own className to reference it in its styles?
There was a problem hiding this comment.
Removing the hardcoded .wordpress-logo would be a breaking change, although I'm ok with it if communicated well.
Would that component be expected to pass its own className to reference it in its styles?
Yes, that's my expectation
Thank you for asking this, I hadn't fully considered that part. I took some time to investigate a bit more: The So as of today, a consuming project already needs to handle CSS/SCSS concerns themselves. We can assume this will be Though, I definitely think we should consider shipping the public npm package with SCSS and CSS Modules preprocessed, at least. I understand we may want to leave flexibility in how consuming projects want to handle the compiled CSS though (injection, extraction, etc). (Needless to say, if we were to start using CSS Modules in @Automattic/team-calypso Does this sound reasonable? Requiring that consuming projects handle CSS Modules on their own for now? And that we should consider Webpack bundling the published Whatever we decide, I can add it to the package readme. |
There was a problem hiding this comment.
Thanks for working on it, Lena!
@Automattic/team-calypso Does this sound reasonable? Requiring that consuming projects handle CSS Modules on their own for now? And that we should consider Webpack bundling the published @automattic/components package, sooner or later?
I'd be curious to see what that looks like before I can tell. The existing use in Jetpack might be a good case to consider:
Ideally, it should be quite straightforward to use the package as part of the DS, with as few steps as possible.
|
Should we update the CSS guidelines of Calypso to clarify the new approach? |
Eventually, yes, but I'm still a bit hesitant to encourage a widespread roll out just yet. I'm hoping we can get a feel of the best practices first in a more contained/controlled scope. |
|
I'd personally feel safer if the package just shipped its built CSS styles, thus making its internal stack irrelevant to consumers. Is that something completely out of the picture? |
There are levels to "building CSS styles":
The current I don't think it's beneficial for And my argument here is that most modern build tooling that is set up to consume a Level 1–2 components package can basically consume a Level 0 package, often with no configuration changes. I am totally fine with bumping |
|
Got it. I think I'd be ok with taking the laziest approach (ie. lowest ranking in Lena's scale) that won't break the bulid process of the projects consuming this package — ie. if we can get away with 0 and projects like calypso / jetpack / woo analytics / etc will continue to build correctly, then we can start with 0. |
|
From my perspective, any package shipped to npm needs to be pre-built using standard technologies. An npm consumer shouldn't have to know how a package is developed internally to be able to use it. Now, how the package is pre-built is a decision the package builder does. So I think we should:
The ideal output is that a consumer doing For '@wordpress/components', we accepted the fact that CSS is built separately from JS and asked people to load the CSS manually. The reason is that WordPress has its own style management flow, so we need the CSS file to be separate. But this is not ideal as well. I guess what I'm saying is that yes, we should do Level 2 like suggested by @mirka
It would make a difference, just today I saw a message on Slack that Jetpack is not building properly because SASS versions conflicts between what is expected from |
Good point, this will likely happen with deprecations like #101983 too. I'll put this on hold until we figure out the build (ARC-170). |
|
I think it's related to this issue of this PRT that tries to import the |
|
Good questions to ask are:
Both questions are two sides of the same coin: extra flexibility can place extra demands on the SCSS consumer. And if there is no flexibility provided, or if the flexibility is not used, we only have extra demands/costs and nothing in return. |
Closes ARC-85
Proposed Changes
This lays the foundations so we can start using CSS Modules for new components added to
@automattic/components.Webpack already supports this, so all we need to add is some type declarations for better devex. I converted
WordPressLogoas a simple example — we can't actually remove the problematicwordpress-logoclass until the existing usages are addressed.An IDE extension like this is recommend for autocompletion and go to definition. I would've liked to also add
typescript-plugin-css-modules, but we're blocked by this limitation.Why are these changes being made?
Non-obscured CSS classes have been problematic as they encourage consumers to override inner styles, making them fragile to changes. We also see name collision issues due to poor namespacing. CSS Modules also tend to be safer for AI-assisted coding since we don't have to worry about name collisions.
Testing Instructions
yarn components:storybook:startand check the WordPressLogo story. The classname containing the styles should be obscured, while thewordpress-logoclassname is kept for back compat.Pre-merge Checklist