Components: Add styles package for new style system
#30630
Conversation
sarayourfriend
requested review from
ajitbohra and
chrisvanpatten
as code owners
sarayourfriend
added
[Feature] Component System
sarayourfriend
force-pushed
the
add/styles
branch
from
c4eeef4 to
f75330f
Compare
|
Size Change: +41.9 kB (+3%) Total Size: 1.47 MB
ℹ️ View Unchanged
|
| /** | ||
| * Internal dependencies | ||
| */ | ||
| import { get } from '../../create-styles'; |
There was a problem hiding this comment.
@jasmussen, who could help reviewing all those config values proposed here?
There was a problem hiding this comment.
To be quite honest, I don't recognize that many of them. The colors I do recognize seem to be quite out of date with the colors defined in our mixin, which are these:
Not only that, but there are numerous additional colors which appear to not be used anywhere, have been created in isolation, don't sync up with the G2 designs from #18667, or even the the colors proposed for the current default color schemes. If we do start using them, it will simply fragment and dilute the interface. I even see the "Inter" font introduced. It's not used anywhere. To frame it differently, anything we introduce should be based on the designs in #27331, and I'd appreciate us not introducing any fonts, colors, shadows or treatments not outlined there.
| @@ -0,0 +1,10 @@ | |||
| export * from './core'; | |||
There was a problem hiding this comment.
While it's convenient to re-export everything, I think we need to have better control over what goes into the public API in the future. I noticed this pattern used quite often in the ui folder. The biggest issue I foresee is that new methods can get exported only to use with tests but through the chain of catch it all re-exports they might suddenly become available for usage outside of the package.
See https://github.com/WordPress/gutenberg/blob/trunk/packages/components/src/index.js for reference.
Sometimes it's convenient to export everything like in @wordpress/blocks:
gutenberg/packages/blocks/src/index.js
Line 12 in b620c92
but then we are explicit one level deeper:
https://github.com/WordPress/gutenberg/blob/trunk/packages/blocks/src/api/index.js
| * Internal dependencies | ||
| */ | ||
| import { get } from '../../create-styles'; | ||
| import colorize from 'tinycolor2'; |
There was a problem hiding this comment.
Just syncing up with @ajlende here, as I think tinycolor is also used for some of the Duotone stuff.
|
Okay, after feedback about variables I've split them into two categories: Theme variables, accessible as CSS Custom Properties via the Theme tokens, accessible only through the |
|
@youknowriad @jasmussen @gziolo @mtias Could y'all take a look at this PR and let me know what all is left for it to be mergeable? Thanks! 🙂 |
|
My primary concern is adding a slew of visual styles that aren't used yet, and that feedback appears well addressed, leaving only a code review. One remaining question, though: does it make sense to introduce "dark mode" when none of the rest of WordPress is able to take advantage of it yet? If at all possible to omit also that, it seems like that would be the leanest version. |
|
|
||
| ## Theme Tokens | ||
|
|
||
| Tokens are similar to variables except that they are not exposed as CSS Custom Properties. This is to avoid introducing too many new public APIs. Instead, we simply access them off of the `ui.tokens` object as you would any normal JavaScript object. `ui.tokens` is a _frozen_ object so runtime changes are no allowed to it, updates to it must be constant and done in the [`styles/tokens.js`](./tokens.js) module. |
There was a problem hiding this comment.
So this does remove the possibility to implement the existing G2 themes right? since themes can't change these variables?
just to clarify for me, this is fine as a start but this is where I think we potentially need a ThemeProvider that relies on "context" in order to support this kind of themes, and at that point, it becomes clear that there's no much difference between "tokens" and "configuration" since both could be defined similary, the value of the configs can still be "CSS variables" while the values of the "tokens" can be just static values.
There was a problem hiding this comment.
Personally I think we should revert back to the existing solution and mark all the variables introduced by the new style system as unstable by prefixing them with --wp-unstable-token or something like that so that no one depends on them with the expectation that they won't change from under them.
I think this is best because it aligns with the original vision of the style system as relying solely on CSS custom properties, allowing theming through the original theme provider without introducing React context into the question which is a huge performance gain (if you're calling useTheme from every component in the tree this creates a significant performance hit that can be avoided just by using CSS custom properties).
What do you think, can we go back to the original implementation (with less variables of course) if we just mark the variables as unstable so we're not introducing a maintanence nightmare scenario?
There was a problem hiding this comment.
What do you think, can we go back to the original implementation (with less variables of course) if we just mark the variables as unstable so we're not introducing a maintanence nightmare scenario?
I don't mind that personally yeah but I guess right now there's a big gap between what a "theme" is for G2 (hundreds of variables) and what a "theme" should be in my mind (a dozen variables at best). Do you think reconciling the two is possible?
There was a problem hiding this comment.
I mean we've already removed the hundreds of variables and paired it down to just a few dozen. I think we should expand our definition of theme to include spacing and some other things important to the "fit" and "feel" of the components that could be really valuable to be manipulated by a consumer through the ThemeProvider API... but the primary concern for themes is colors and fonts, which I think we're achieved now.
If we could move the tokens back into the regular theme now that would solve this concern for me.
What do you percieve to be the gap at this point in the definition of what a theme is, now that we've removed almost all the variables?
There was a problem hiding this comment.
What we have now still feels a bit too much for me. I think G2's original goal was trying to allow "any theme" for any kind of design while I feel a WP focused component library should be more opinionated while allowing some customizations. It is important to think of a "theme" in this context as a "theme" for the entire WP-Admin and not just the component library.
In a lot of ways G2 was built as multi-purpose component library for any kind of JS frontends, similar to the number of components libraries that exist in the wild. This vision is where I feel there's a contradiction with what we want to achieve: a coherent component library for WordPress. So opinions are fine and even good and customization should be minimal IMO.
Based on this, for me the theme should be:
- A main color
- Potentially a secondary color
- A grid unit (space)
a font family and font size, maybe but even that I don't think it's necessary for us (at least not at first).
I'm not sure I understand what you mean by "fit"/"feel".
There was a problem hiding this comment.
Also what makes me uneasy about what G2 does at the moment is that it was develop as its own project and had to make a lot of decisions with designers from WordPress being involved in these discussions about what "theme" we want to support and what should be opinionated.
There was a problem hiding this comment.
By fit and feel I just mean spacing, border radius, border colors, background colors for elements that sit on top the background surface, intermediary colors like modal backdrop colors, animation variables (which are especially useful for automatically handling reduced motion instead of having to duplicate reduced motion handling across all components... or removing animations altogether if that's the target use-case needs it)... these are just some of the things I think fall into the fit and feel of a component system's design system or theme.
I don't think we need all 300 or whatever colors we had before, but I do think some of the things we've dropped are important to maintain for a component system to be customizable.
Say for example the WooCommerce plugin wanted to use WordPress/components, they'd need to be able to customize almost all of these things for wp/components to fall inline with their pre-existing design language.
There was a problem hiding this comment.
WooCommerce plugin should customize the library to included its branding but it shouldn't be a departure from what the core styles are. This is where the different in vision stands, in the level of customizable things we want to have. I know @jasmussen has thoughts on this as well. It's clear to me for instance that the level of customizability G2 has currently is just way too much.
There was a problem hiding this comment.
There is an important balance to strike regarding how much is customizable. Too little and it might be a pain point, too much and it risks lacking character and direction. Ultimately that isn't something to decide completely, but I find it easier to start with less and add when it becomes necessary, rather than start with total customizability and realize we need to reel it in.
There was a problem hiding this comment.
Gotcha. If it's currently way too much then that's just how it is, let me know how best to pare it down and I'm happy to do so.
Personally I have no vision for this library, I just want it integrated so we can take advantage of the CSS-in-JS offerings it provides. I'm not married to any of the implementation details aside from ones I think give us a performance or usability edge over using raw Emotion.
The way this style system handles themes is not one of those things (aside from the performance aspect I pointed out of not wanting to use React context to pass around theme variables, that I think we should keep in CSS custom properties with the unstable prefix).
| @@ -0,0 +1,34 @@ | |||
| # Components | |||
|
|
|||
| ## StyleFrameProvider | |||
There was a problem hiding this comment.
This is great but I wonder if we could achieve the same without another provider, I mean why can't the style system directly relies on ownerDocument or something instead of requiring this provider.
There was a problem hiding this comment.
I'm not sure I follow. Can you elaborate more on your alternative solution to iframe handling?
There was a problem hiding this comment.
I don't know the system enough how the current system works to propose anything meaningful. I guess my thinking is why can't a component just inject its own styles into the frame where it's used. This is just a random thought now, not important for this PR.
| - `tokens`: A pre-defined set of theme variables, accessible via the frozen `tokens` object. See [Theme Tokens](#theme-tokens) below. | ||
| - Various mixins. See [Mixins](#mixins) below. | ||
|
|
||
| ## Theme Configuration |
There was a problem hiding this comment.
Now that we're only keeping the variables that are already defined in our Sass files as theme configuration, does this mean that we can now remove all the code that generates the CSS variables from create-styles like discussed in the previous PR?
There was a problem hiding this comment.
No because the Sass variables are just Sass variables, they're not CSS custom properties so they're not accessible from CSS-in-JS. If anything I'd say we should remove the Sass variables and switch to just CSS custom properties?
There was a problem hiding this comment.
No I'm not talking about SASS variables, I'm talking about the CSS variables generated by the SASS mixin we already use to theme the components and UI.
There was a problem hiding this comment.
Gotcha. Currently that's only like one variable right?
We already declare a dependency on base-styles anyway so I don't see that as a problem 🤔 But if we changed the style system to use the unstable prefix I think we should keep it consistent and keep all the style system variables inside the style system and not rely on anything external to generate the variables, right?'
There was a problem hiding this comment.
I think the current system in create-styles that generates the variables on load from the theme is a bit too advanced for our needs. I don't really care whether we simplify that system and make it the canonical way of including the "default css variables" or whether we just enhance our existing mixin to include the new variables, but we should just have just one consistent system and use it across our packages. (components package not being the only package that can be themable and React dependency shouldn't be mandatory)
There was a problem hiding this comment.
To clarify, I don't think this is necessarily something that we need to act upon on this PR necessarily but something that need to be solved.
|
If @jasmussen and @youknowriad are happy then I'm happy, too 😄 I don't have any concerns myself. |
Description
Part of #30503
This PR adds the
stylepackage from G2. This package contains the theme as well as some simple helper functions. It also further enhances thecssfunction's responsive styles by making them fall in line with the style system by enhancing the call toresponsivewithgetScaleStyles. This means that responsive values will be transformed using thespacefunction which means responsive values like[2, 4, 6]will all get passed tospace, making them fall in line with the style system's spacing (🎉)It also removes the IE11 shim for supporting CSS Custom Properties fallbacks by removing it entirely. I decided to do this becuase the RootStore method was causing a difficult/impossible to debug infinite loop for some reason and instead of spending hours debugging something that is going to be removed soon, I decided to just go ahead and remove it.
This PR also adds a number of unit tests that test both
stylesandcreate-styles, mostly around thecssstyledand component interpolation functions.Note: to resolve an ESLint warning about direct window access, I had to change the
useResponsiveValuefunction to accept anodeparameter. We'll have to update it's usages to pass a node when we update existing components to use this version ofstyles.Outstanding question: How to resolve and handle theme variables? Should we move the theme initialization into a common package that this then uses? Now that you can see more clearly how the theme is initialized, are there any ideas for how to clean this up and make it more comprehendible? Should we indeed remove all the various extra generated color variables as was suggested? Should we go ahead and convert non-color variables into regular JS object key/value objects (or do that in a fast-follow PR [my preference])?
How has this been tested?
Unit tests pass as well as type checks.
Types of changes
New, unexported and unused features.
Checklist:
*.native.jsfiles for terms that need renaming or removal).