Improve CSS setup instructions in package readmes#76975
Conversation
|
|
||
| ### Within WordPress | ||
|
|
||
| To ensure proper load order, add the `wp-components` stylesheet as a dependency of your plugin's stylesheet. See [wp_enqueue_style documentation](https://developer.wordpress.org/reference/functions/wp_enqueue_style/#parameters) for how to specify dependencies. |
There was a problem hiding this comment.
@wordpress/dataviews and @wordpress/admin-ui don't have a stylesheet handle, so there's currently no reliable way for a third-party WP plugin to load the package styles without potentially loading duplicate or conflicting styles on a given route. (If something else is loading the same package styles on the same route.)
Maybe that's fine, but mentioning here for completeness (cc @oandregal).
|
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. |
|
Flaky tests detected in b8eb232. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/23949739516
|
aduth
left a comment
There was a problem hiding this comment.
Some minor comments, but LGTM overall 👍
packages/admin-ui/README.md
Outdated
| npm install @wordpress/admin-ui --save | ||
| ``` | ||
|
|
||
| ## Set up |
There was a problem hiding this comment.
Super nit: But I kept wondering if this should be "setup" or "set up", but in turn it made me think it's not a very common heading I see in READMEs, compared to something like "Getting started", "Usage", maybe "Setup instructions", etc.
There was a problem hiding this comment.
Although in the case of components package here, it already has "Usage". And we're treating this as something different 🤔 Maybe it's fine to have a separate section for this "necessary steps between installation and usage". Or we could fold it into an existing section. "Setup" is probably more consistent as a noun heading though, if we kept it.
There was a problem hiding this comment.
Good point about the noun/verb agreement. Turns out "Setup" is used as a heading in some other packages in here as well, so I went with that ecf4fd6.
| import '@wordpress/components/build-style/style.css'; | ||
| import '@wordpress/admin-ui/build-style/style.css'; |
There was a problem hiding this comment.
Aside: It would be really nice if we made a subpath export with a nicer import path here, to avoid build-style being part of the import path. Feels like an implementation detail we shouldn't need to expose to end-users 🤷
There was a problem hiding this comment.
On the other hand, it's probably a pretty niche use case to use these libraries outside of WordPress?
There was a problem hiding this comment.
True, not an especially urgent problem. More to the general consistency of how we want to use subpath exports. They're a public API. But build-style means nothing and could disappear or be renamed tomorrow if we wanted it to.
There was a problem hiding this comment.
it's probably a pretty niche use case to use these libraries outside of WordPress?
Note that until admin-ui is bundled by default by tooling provided by Gutenberg, the import is mandatory also in WP Admin contexts.
It might even help clarify in docs that component style is required to import only outside WP contexts, and the other two in all contexts?
There was a problem hiding this comment.
Note that until admin-ui is bundled by default by tooling provided by Gutenberg, the import is mandatory also in WP Admin contexts.
I've raised this as a structural problem for both DataViews and Admin UI. Possibly less of a problem for Admin UI, if there's less chance of stylesheet conflicts on a given route (as opposed to e.g. DataForm, which could more plausibly have multiple separate bundles loading into a single route). I believe the simplest solution for Admin UI would be to switch to our CSS module injection system (#77032).
| import '@wordpress/admin-ui/build-style/style.css'; | ||
| ``` | ||
|
|
||
| RTL versions of the stylesheets are available in the same paths, but with `-rtl` appended to the filename (`style-rtl.css`). The design tokens stylesheet is universal and does not have a separate RTL version. |
There was a problem hiding this comment.
I wonder if it's worth clarifying that we don't ship an RTL-specific stylesheet for the ui package.
I also wonder what level of effort would be involved in removing it from this admin-ui package, since it doesn't include a whole lot and might be easy to migrate to logical properties.
There was a problem hiding this comment.
I wonder if it's worth clarifying that we don't ship an RTL-specific stylesheet for the
uipackage.
The "The design tokens stylesheet is universal and does not have a separate RTL version." line at the end was supposed to be this clarification. Was that unclear or did you mean something else?
I also wonder what level of effort would be involved in removing it from this
admin-uipackage, since it doesn't include a whole lot and might be easy to migrate to logical properties.
Oh yes, should be pretty easy especially at this early stage. Also a move to CSS modules. In light of the #76744 problem too, I'm thinking we can enforce the "CSS modules + logical properties" combination repo-wide on packages that want to adopt this new setup we already do 😄.
There was a problem hiding this comment.
The "The design tokens stylesheet is universal and does not have a separate RTL version." line at the end was supposed to be this clarification. Was that unclear or did you mean something else?
I was more referring to a note in packages/ui/README.md for someone who might be confused about the absence of an RTL stylesheet. There's an implication here that downstream consumers will be handling RTL concerns, so if I'm an app developer and I know that I need to handle RTL, I might be confused if I can't find any mention of a stylesheet I need to load for RTL in @wordpress/ui documentation.
There was a problem hiding this comment.
Issue for CSS module migration of admin-ui package: #77032
|
Thanks for the changes, this reads a lot better. |
* Improve CSS setup instructions in readmes * Fix * Remove non-existent stylesheet handles * Use Setup as readme section heading * Add note to ui readme about no RTL version Co-authored-by: mirka <0mirka00@git.wordpress.org> Co-authored-by: aduth <aduth@git.wordpress.org>
What?
Adds a Set up section to the readmes for
@wordpress/admin-ui,@wordpress/components,@wordpress/dataviews, and@wordpress/ui, covering stylesheet dependencies for WordPress plugins and for apps outside WordPress.Why?
These packages depend on CSS from themselves and/or other packages. Instructions were split across sections or easy to miss, and non‑WordPress setups did not always spell out install steps, import order, RTL paths, or which
wp_enqueue_stylehandles to depend on.How?
wp_enqueue_style).npm installwhere needed andimportexamples for@wordpress/theme/design-tokens.cssand@wordpress/components/build-style/style.css, plus RTL paths for components CSS where applicable.@wordpress/ui: Clarify that WordPress loads styles for you; outside WordPress, recommend design tokens for full theming and add a short note aboutisolation: isolateon the layout root for portaled popovers.Testing Instructions
See that the instructions are correct and make sense.
Use of AI Tools
Composer 2 was used to draft this PR description.