Components: Add create-styles #30509
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
sarayourfriend
requested review from
ajitbohra and
chrisvanpatten
as code owners
sarayourfriend
added
[Feature] Component System
|
Size Change: +2.23 kB (0%) Total Size: 1.42 MB
ℹ️ View Unchanged
|
youknowriad
reviewed
Apr 5, 2021
There was a problem hiding this comment.
Thanks for starting this PR. I didn't do a full review but I have some questions.
Overall, I'm trying to see if we can iterate and simplify things. I'm basically trying to understand how the theme provider works and questioning why we need all these things :P. At first sight it looks way more complex than what I have imagined.
packages/components/src/ui/create-styles/components/theme-provider/theme-provider-context.js
Outdated
Show resolved
Hide resolved
packages/components/src/ui/create-styles/components/theme-provider/theme-provider-context.js
Outdated
Show resolved
Hide resolved
packages/components/src/ui/create-styles/components/theme-provider/theme-provider-context.js
Outdated
Show resolved
Hide resolved
packages/components/src/ui/create-styles/components/theme-provider/theme-provider-context.js
Outdated
Show resolved
Hide resolved
packages/components/src/ui/create-styles/components/theme-provider/theme-provider.js
Outdated
Show resolved
Hide resolved
packages/components/src/ui/create-styles/components/theme-provider/theme-provider.js
Outdated
Show resolved
Hide resolved
|
Thanks @youknowriad. After our discussion I've deleted the ThemeProvider and the duplicated hooks 🎉 |
youknowriad
reviewed
Apr 6, 2021
packages/components/src/ui/create-styles/create-compiler/create-compiler.js
Outdated
Show resolved
Hide resolved
packages/components/src/ui/create-styles/create-compiler/create-compiler.js
Show resolved
Hide resolved
packages/components/src/ui/create-styles/create-compiler/plugins/index.js
Show resolved
Hide resolved
packages/components/src/ui/create-styles/create-style-system/README.md
Outdated
Show resolved
Hide resolved
packages/components/src/ui/create-styles/create-style-system/create-core-element.js
Show resolved
Hide resolved
packages/components/src/ui/create-styles/create-style-system/create-styled-components.js
Show resolved
Hide resolved
packages/components/src/ui/create-styles/create-style-system/generate-theme.js
Show resolved
Hide resolved
| * @param {boolean} isGlobal | ||
| * @return {string} Compiled innerHTML styles to be injected into a <style /> tag. | ||
| */ | ||
| export function transformValuesToVariablesString( |
There was a problem hiding this comment.
This sound like things used by the ThemeProvider to generate the CSS variables, since we don't have that anymore, maybe there are some things to clean/remove here?
There was a problem hiding this comment.
Nope, this is used by generateTheme to apply the generated theme variables to the :root scope at the very least as well as the different "modes" offered by the new style system.
There was a problem hiding this comment.
so basically, this is just a duplication of what we currently do in this mixin https://github.com/WordPress/gutenberg/blob/trunk/packages/base-styles/_mixins.scss#L441
I think we should just add the G2 variables there for now and avoid all the extra JS logic that runs at init.
There was a problem hiding this comment.
But aren't we trying to do CSS-in-JS? It seems counter intuitive that we would keep our variables in SASS in that case?
There was a problem hiding this comment.
There's a few things here:
- I think the logic of these function is just too complex for what it is right now, why not just inject a static
<style>element? - I'd like us to avoid having two ways of injecting these "theme styles", so we should consolidate in one way
- Right now the variables that constitute a "theme" are used not just in the "@wordpress/components" package, they can be used in all the other packages and these packages are not CSS in JS.
There was a problem hiding this comment.
I'd love other thoughts on this as well. This is something that can have some impact long term (WP user profiles, theming components) so yeah more thoughts and opinions here would be good.
I lean towards playing nicely with existing systems and starting small. As Riad points out, the impact of these changes is unclear, and there's the risk of starting de facto APIs that will later cost us to support.
There was a problem hiding this comment.
What if we change to prefix to --wp-experimental for all these variables, leave the rest the way it is, and then re-evaluate how to do this once we've integrated the styles package which actually contains the theme? It might be more helpful to brainstorm solutions at that point than now when we only have half the context.
There was a problem hiding this comment.
I would defer to Riad or Miguel, whose concerns I share: we should move carefully and only introduce components, systems, and custom properties when we know for a fact that they will be needed and used.
To use an example: color custom properties. I've seen a tendency to design big palettes and adding them all as custom properties to be conveniently available. In practice, they end up being dead weight on most pages, and causing developer confusion with the breadth of choice (maybe gray-200 was chosen when gray-300 should've been used).
That is to say: these properties are only ever useful if they will be used, and ideally used often enough to deserve such stature. I do recognize that it's a spectrum ranging from overwhelming palettes to lack of flexibility, and that the ideal spot, probably, is somewhere in the middle. But it is an argument for moving slowly and only when we have some confidence in the system.
There was a problem hiding this comment.
@wordpress/base-styles contain several files and a huge amount of code. Ensuring it stays in sync with CSS-in-JS bits might be a huge challenge unless we find a good way to share them.
By this do you mean that the new package would just create the theme variables into the :root scope?
This sounds like a reasonable proposal because we could use those variables in other places – outside of @wordpress/components.
There was a problem hiding this comment.
What if we change to prefix to --wp-experimental for all these variables, leave the rest the way it is, and then re-evaluate how to do this once we've integrated the styles package which actually contains the theme? It might be more helpful to brainstorm solutions at that point than now when we only have half the context.
That's fine I guess but it's definitely something we need to address.
packages/components/src/ui/create-styles/hooks/use-hydrate-global-styles.js
Show resolved
Hide resolved
gziolo
reviewed
Apr 6, 2021
packages/components/src/ui/create-styles/create-compiler/README.md
Outdated
Show resolved
Hide resolved
youknowriad
reviewed
Apr 6, 2021
|
After the discussions above I've:
|
youknowriad
approved these changes
Apr 8, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
The first step of #30503.
How has this been tested?
There are minimal run-time changes from the
create-stylesin G2:isusageutilsinto the package itself (basically getting rid of imports from@wp-g2/utils)Otherwise most of the changes are related to type issues that popped up for one reason or another and were fixed here.
Types of changes
Adding the
create-stylesfolder topackages/components/src/ui. This is a non-breaking change and, in fact, the new stuff isn't used anywhere yet so there's zero risk to this PR.Checklist:
*.native.jsfiles for terms that need renaming or removal).