-
Notifications
You must be signed in to change notification settings - Fork 2k
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
calypso-build: remove dependency on calypso-color-schemes #36471
Conversation
This PR does not affect the size of JS and CSS bundles shipped to the user's browser. Generated by performance advisor bot at iscalypsofastyet.com. |
…ts define that explicitly
57c258d
to
8e62cfa
Compare
Rebased, resolved conflicts and fixed a little bug that caused several builds to fail (destructuring and option from an undefined |
I'm a bit worried about this change, specifically about its implications for Requiring users to add a webpack config and to use the SASS preset with a custom option to inject It was me who coded the Webpack presets concept in their current shape, but I'm not particularly proud of it, and I don't think it's a great API to expose for less-advanced users (while it might be okay for Calypso's own Webpack config, which is just a different level of complexity) -- to some extent, it is "our own obscure abstraction layer/config language". This is a recurring problem with the intended 'zero-but-incremental' (Peano? 😄 ) config system in a world where different small pieces need to be configured, and there is no homogenous standard for all of them (Babel presets are pretty much the nicest thing we have). I think I'd still prefer something that's closer to All that goes to say: Can we modify |
That's a fine and I agree with the sentiment but a bunch of CSS color variables, many specific only to Calypso (think Sanity check fails especially when trying to update Moreover, there can be projects that should use a completely different set of colors (think e.g. Happy tools). Shipping colors by default is a blocker for such adoption. The solution is to either not include the color variables and let consumers keep up with a specific version of Thoughts? |
module.exports = ( { options: { preserveCssCustomProperties = true } } ) => ( { | ||
module.exports = ( { | ||
options: { preserveCssCustomProperties = true, importCssCustomPropertiesFrom }, | ||
} ) => ( { | ||
plugins: { | ||
'postcss-custom-properties': { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could postcss-custom-properties
plugin be left out if importCssCustomPropertiesFrom
is falsey? That would effectively make this opt-in.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@simison postcss-custom-properties
is useful even in the default configuration when neither of the "preserve" and "import from" options are passed. It then converts:
:root {
--color: red;
}
h1 {
color: var( --color );
}
to
:root {
--color: red;
}
h1 {
color: red;
color: var( --color );
}
It inlines the default variable as a fallback. Think of it as a IE11 polyfill.
The "import from" option is needed only when the variables are defined in a different stylesheet. node-sass
and postcss
process stylesheets one by one, exactly as they are stored in sources. Only at the very end does webpack merge them together.
I think I might've found the missing piece: https://github.com/postcss/postcss-loader#path We should be able to do something similar to
|
Implementation notes for #36471 (comment):
|
Going to take a stab at this now. |
|
Removes the
calypso-build
dependency oncalypso-color-schemes
and lets projects that use both packages configure the needed CSS bits explicitly.Fixes #36466.
calypso-color-schemes
colors prelude from the default webpack config. That was useful when the colors were defined as SCSS variables, but now when they are defined as CSS variables included in the CSS output, the prelude no longer makes senseimportFrom
declaration frompostcss-custom-properties
default config and replaces that with configurableimportCssCustomPropertiesFrom
option for theSassConfig
preset.importCssCustomPropertiesFrom
option.Let's see how this works with other projects that use
calypso-build
.A little reminder about what
postcss-custom-properties
does: when there is a CSS rule that uses a CSS variable:it has one problem: it doesn't work in IE11 and other old browsers without CSS variable support.
postcss-custom-properties
adds a "polyfill" here. Withpreserve: false
, it inlines the variable:With
preserve: true
, it preserves the CSS variable and adds a fallback definition:Browser with CSS variable support uses the version with a variable (that can be overridden with a different value, depending on active CSS classes), IE11 uses the fallback.
importFrom
option specified a module where to import the CSS variables from. When compiling modularized CSS with webpack, the CSS variable definitions are in a different compilation unit (the entrypoint) and the compiled CSS doesn't see them. They only come together after webpack bundles all the pieces together.