DataViews: externalize theme stylesheet#75182
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: -17 kB (-0.57%) Total Size: 2.98 MB
ℹ️ View Unchanged
|
| import fieldsRtl from '../package-styles/fields-rtl.lazy.scss?inline'; | ||
| import mediaFieldsLtr from '../package-styles/media-fields-ltr.lazy.scss?inline'; | ||
| import mediaFieldsRtl from '../package-styles/media-fields-rtl.lazy.scss?inline'; | ||
| import theme from '../package-styles/theme.lazy.scss?inline'; |
There was a problem hiding this comment.
I would like to consolidate this to a single designTokens import.
There was a problem hiding this comment.
👍 designTokens looks good to me as name, though that link points to the UI stylesheet that is different? I merged in the interest of velocity, but happy to iterate on this or address any other issue if you can't on #74899
|
|
||
| ## Stylesheet Dependencies | ||
|
|
||
| DataViews depends on stylesheets from `@wordpress/components` and `@wordpress/theme`. In a WordPress admin page context, these are loaded automatically. For applications outside WordPress, you will need to include these stylesheets: |
There was a problem hiding this comment.
Sorry I overlooked this point — as far as I know these are not "loaded automatically" in WordPress, they require an wp_enqueue_style like we document in the @wordpress/components readme:
gutenberg/packages/components/README.md
Line 28 in b054863
This is also wrong in the @wordpress/ui readme so I'll fix.
Another point is that I don't think we load the @wordpress/theme design tokens stylesheet in Gutenberg yet, so if DataViews actually uses anything that requires a design token, that might be broken right now 😅
(cc @WordPress/gutenberg-components in case I got something wrong here)
There was a problem hiding this comment.
I thought this would be handled through the dependency chain from DataViews style loading to the wp-theme stylesheet as a dependency, i.e. what was corrected in #74743.
There was a problem hiding this comment.
Even if the DataViews stylesheet did load the theme stylesheet (which it doesn't anymore in this PR), shouldn't a consumer need to wp_enqueue_style the DataViews stylesheet at least? To ensure correct load order, at the very least? That's what's documented in the @wordpress/components readme, so if that's wrong I would want to fix it.
There was a problem hiding this comment.
Hm, the more I dig into this, the more I feel we may in-fact be missing something.
I think there's some partial truth to this. There are examples like @wordpress/boot where technically WordPress is handling the "loaded automatically" by loading theme styles. But obviously this boot package isn't yet used everywhere.
There may be some incidental behavior where the theme styles are brought in via dependency chains, e.g. @wordpress/dataviews -> @wordpress/ui -> @wordpress/theme which is translated into style dependencies through the build's inferStyleDependencies. But it builds those dependencies out of imports in the JavaScript code, which is fragile and not a strong signal of what style dependencies should be loaded.
How it should work? I'm not super clear, but maybe one of:
- Use
@wordpress/bootin more places as a true boot-point for "WordPress" and all its core styling - Manually revise dependencies like what we do in
lib/client-assets.phpto includewp-themein more entrypoint packages - Sprinkle
@use "@wordpress/theme/src/prebuilt/css/design-tokens.css"throughout some more entrypoint stylesheets - Invent a way to more explicitly define these dependencies, like a
wpStyleDependenciesfield inpackage.jsonthat's respected by the build tool
Some of those options may still require documentation that consumers would be responsible for ensuring the styles are enqueued.
There was a problem hiding this comment.
The way I see it is:
- is any package that renders
<ThemeProvider>should be also doing@use "@wordpress/theme/src/prebuilt/css/design-tokens.css" - The "boot" package is one of many entry points in WordPress
- Most edit-* packages are also entry points and if we want to start using UI components in these, they need to render
ThemeProviderand include the tokens CSS. - Since dataviews is already using UI components and dataviews is used in most of these edit-* packages, we need to follow-up with the addition of
ThemeProviderand tokens to all of these packages.
Where I think we could improve things:
- Using
ThemeProvidershould ideally automatically inject the design tokens (once).
There was a problem hiding this comment.
- Using
ThemeProvidershould ideally automatically inject the design tokens (once).
It's an option, though as discussed previously in #74174 (comment) it feels like it just shifts the dependency from the a stylesheet to a React component. So a project external to WordPress that wants to use DataViews but doesn't care about theming still has to introduce a ThemeProvider wrapper somewhere.
- Most edit-* packages are also entry points and if we want to start using UI components in these, they need to render
ThemeProviderand include the tokens CSS.
Yeah, I think this is a logical next step. We may not strictly need the ThemeProvider component, just the styles should be enough to start. I also want to be conscious whether this ships with WordPress 7.0 and creates expectations around the theme variables; ideally we'd leave it out, but I also don't want anything to be broken if it was by these changes 😓
If we did add the styles to the entrypoints, then I think it's accurate to say that this would be handled automatically inside WordPress as mentioned in the changelog note.
There was a problem hiding this comment.
It's an option, though as discussed previously in #74174 (comment) it feels like it just shifts the dependency from the a stylesheet to a React component. So a project external to WordPress that wants to use DataViews but doesn't care about theming still has to introduce a ThemeProvider wrapper somewhere.
Yes, but the main API for these components is React, so it's just simplifies things and ThemeProvider is almost already mandatory. That said, this is not the main thing in this discussion. We can live with or without it.
Yeah, I think this is a logical next step. We may not strictly need the ThemeProvider component, just the styles should be enough to start.
We do need the ThemeProvider (or more precisely the UserThemeProvider) in order to adapt to the currently selected user profile.
If you think this is too soon for 7.0, the only other option I see is to revert the usage of the UI package in dataviews.
There was a problem hiding this comment.
We do need the ThemeProvider (or more precisely the UserThemeProvider) in order to adapt to the currently selected user profile.
I'm not sure if we're using anything where we need the user preference at the moment? Although yes, we should probably just go ahead and get this established.
If you think this is too soon for 7.0, the only other option I see is to revert the usage of the UI package in dataviews.
I was going to suggest we could revert the changes made in this pull request, but (a) I'm still not entirely sure it's broken as-is, and (b) regardless, the styles would be defining :root theme CSS properties, so those are going to be present for everyone.
I think we could probably be okay with what's shipping, especially after the latest rounds of token refinement. I'd like if we had some flexibility for revisions without too much fear of backwards-compatibility even after releasing, though I dunno how others may feel about that.
There was a problem hiding this comment.
As of now, ThemeProvider is only needed if in need of customizing the default theme — otherwise, the CSS file includes all the required variables with the default theme values.
I see value in not requiring ThemeProvider, and also in the fact that using CSS variables should prevent FUOC while the ThemeProvider hasn't calculcated the overridden CSS variables.
There was a problem hiding this comment.
Important thing is that edit-site and edit-post... still update properly the colors when the user profile is modified. So if tomorrow someone starts using any component that uses colors from @wordpress/ui in @wordpress/dataviews we'd be introducing a bug without knowing.
Co-authored-by: oandregal <oandregal@git.wordpress.org> Co-authored-by: mirka <0mirka00@git.wordpress.org> Co-authored-by: aduth <aduth@git.wordpress.org>
Co-authored-by: oandregal <oandregal@git.wordpress.org> Co-authored-by: mirka <0mirka00@git.wordpress.org> Co-authored-by: aduth <aduth@git.wordpress.org>
Fixes #74594
What?
Remove the
@wordpress/theme/design-tokens.cssstylesheet from dataviews.Why?
This can lead to duplication of the stylesheet the design tokens are already provided elsewhere in their application.
How?
Testing Instructions