UI: Add Skeleton component#79671
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 Unlinked AccountsThe following contributors have not linked their GitHub and WordPress.org accounts: @AyushMhaisane. Contributors, please read how to link your accounts to ensure your work is properly credited in WordPress releases. 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. |
| component: Skeleton, | ||
| parameters: { | ||
| componentStatus: { | ||
| status: 'use-with-caution', |
There was a problem hiding this comment.
Setting as use-with-caution for now. We can get feedback, iterate if necessary and mark this as ready later.
There was a problem hiding this comment.
In that case, let's remove the manifest tag. I'm also ok with adding the component to the use-recommended-components allow list so we can start using the component without eslint disables.
|
Size Change: -10 B (0%) Total Size: 7.63 MB 📦 View Changed
|
|
Flaky tests detected in d29b9ad. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/28646314351
|
There was a problem hiding this comment.
I'm always torn about adding a Skeleton component, since it feels like legitimizing and inviting slow/lazy-loading patterns. I personally wish we could actually make an effort to build UIs and UX where things load instantly (cached data, optimistic UI updates, etc.) and are invalidated when fresher data comes in later.
But I realize it's not the point of this PR, and that the shift would be profound and require a lot of work in the data pipelines.
| /* | ||
| * 1.5s reads better for a pulse than the motion token scale, | ||
| * which tops out at 400ms (--wpds-motion-duration-xl). | ||
| */ |
There was a problem hiding this comment.
Makes sense, not even sure if we should consider adding this duration to the DS tokens, in case it's an one-off exception? cc @WordPress/gutenberg-design
|
Thanks for the review!
I tend to agree, ideally we wouldn't need this. But given the nature of WordPress and that it can run on very different infrastructures, there are places where loading isn't instant and having a consistent way to handle that improves the UX. Besides that, thinking of extenders, it can be a good thing to have a unified design language for this (or at least, that can be consumed). |
Easy to agree — but this is also a very pragmatic addition. It might make sense — perhaps in another PR — to add "Usage guidelines" for different loaders, including:
And perhaps even elaborate on @ciampo 's point about cached data and optimistic UI updates, combined with That way we're not simply adding more components to the system, but adding design guidance for how to use them. Meanwhile, I'll open a PR to expand a bit on WP Build docs route prefetching, which is also good to document better. (Edit: #79688) |
ciampo
left a comment
There was a problem hiding this comment.
I don't have further blocking feedback at the moment 🚀
Not sure if we want to wait for any other feedback from @WordPress/gutenberg-design or @WordPress/gutenberg-components folks.
| }, | ||
| }; | ||
|
|
||
| export const TextLines: Story = { |
There was a problem hiding this comment.
Idea: if we do intend to use it for text lines, what if we offered a prop to easily match skeleton to take the right height as the corresponging Text variant?
A simpler alternative: use --wpds line height tokens for the Skeleton's height in this example?
There was a problem hiding this comment.
Went with the simpler option, using the line-height tokens.
There was a problem hiding this comment.
Should we generally use --wpds-dimension-size-* tokens where possible?
There was a problem hiding this comment.
Agreed 🫡
Updated stories to use tokens now e7ab6d7
| * 1.5s reads better for a pulse than the motion token scale, | ||
| * which tops out at 400ms (--wpds-motion-duration-xl). | ||
| */ | ||
| animation: skeleton-pulse 1.5s ease-in-out infinite; |
There was a problem hiding this comment.
Would using --wpds-motion-easing-subtle from motion tokens work here?
There was a problem hiding this comment.
Good suggestion, thanks! Gave that a try and even though it's named as subtle, at the scale that skeleton is used it looks more aggressive than ease-in-out. The --wpds-motion-easing-subtle curve ends up feeling like a strong heartbeat rather than a subtle pulse.
Created a codepen to make it visual https://codepen.io/juanfraa/pen/RNKLvpb
Thinking that this can be potentially used for several elements in the screen, I believe ease-in-out is a better fit here.
| import type { BorderRadiusSize } from '@wordpress/theme'; | ||
| import { type ComponentProps } from '../utils/types'; | ||
|
|
||
| export interface SkeletonProps extends ComponentProps< 'div' > { |
There was a problem hiding this comment.
I'm not necessarily opposed to these props, but some KISS thoughts for the first iteration:
- Is the
radiusprop necessary? The consumer will likely always need to write some CSS (at least for the dimensions), so it's not that having a radius prop would remove the need to write any CSS. Seems like the ergonomics wouldn't change much by having consumers use radius tokens directly. - What is the use case for ever disabling the loading animation, or having alternative loading animations? If it's not an immediate need, we can maybe think about adding that prop later.
There was a problem hiding this comment.
Thanks for reviewing!
- I think the radius prop could be handy as we're relying on tokens, and if something changes there we (anyone using the component) wouldn't be having to hunt down hardcoded values on the skeleton usages.
- Good catch. I added the prop in case we wanted to leave space for including other common animations for skeleton (shimmering, for example). I removed it and we can always come back to adding it in case we want to have more animations c31575d
There was a problem hiding this comment.
- if something changes there we (anyone using the component) wouldn't be having to hunt down hardcoded values on the skeleton usages.
What kind of changes are we thinking of though? If anything changes in the tokens, that's a system-wide change. Specifically, if any of the token values change, there's usually nothing that consumers should do. If any of the token keys change (add/remove/rename), that's something we need to address globally in every applicable stylesheet in the codebase, but it's usually just a simple search/replace. It would actually add more migration work if a component's props were tied to a set of token keys, since the bulk replace operation for stylesheets wouldn't cover component props.
There was a problem hiding this comment.
If we're still in doubt about the radius prop, better to start without and add it back if we find a real-world need for it?
It's always easier to add than to remove an API.
We could show expected border radius tweaks via styles and WPDS tokens in Storybook, to start with.
There was a problem hiding this comment.
My thought process is: we have a certain element in the UI with a border radius (let's say a box that contains the preview of a pattern, with md). While that's loading we use the skeleton, with the md border radius. If there's a change in the token value (because of an update, or because there's a new theme, or whatever) there's no maintenance to do. It'll work without having to go and update anything.
If for the same case, you use the Skeleton and rely on an inline style with the equivalent value to the md token, whenever there's a change, you'll have to go back and update those inline styles.
There was a problem hiding this comment.
Oh I see. You were comparing "token prop vs. literal value", I was comparing "token prop vs. token in CSS".
To me this is actually a system-wide API decision: Are we ok with adding token-mirroring props for the purpose of enforcing token adherence? I think there's a similar case to be made in Text, where we currently don't have a token-mirroring color prop.
I fear there's a slippery slope there, but in any case that's perhaps a separate system decision, like @ciampo suggested. My hope is that token adherence is mainly achieved through convention, like how this codebase is already adhering to @wordpress/base-styles Sass variables (especially color) without much active enforcement. But we'll see. We can start without, but it may turn out to be quite clear that a radius prop is necessary for some reason or another.
There was a problem hiding this comment.
Oh I see. You were comparing "token prop vs. literal value", I was comparing "token prop vs. token in CSS".
Exactly. I was thinking more from a consumer standpoint. My assumption is they would probably rely on a t-shirt size rather than knowing (if they do) that there's a variable matching that.
But I might be overthinking it 😅 So it makes sense to remove it and see if it becomes necessary. Removed it here > bb60a79
Thanks again 🙏
There was a problem hiding this comment.
I think we should add some kind of forced-colors support?
There was a problem hiding this comment.
Good catch. Made the changes and took dialog and button as a reference 79adcde
| component: Skeleton, | ||
| parameters: { | ||
| componentStatus: { | ||
| status: 'use-with-caution', |
There was a problem hiding this comment.
In that case, let's remove the manifest tag. I'm also ok with adding the component to the use-recommended-components allow list so we can start using the component without eslint disables.
| import { type ComponentProps } from '../utils/types'; | ||
|
|
||
| export type SkeletonProps = ComponentProps< 'div' >; |
There was a problem hiding this comment.
Not sure if we want to keep this one now that's this thin. I would vote to remove it but haven't seen any other without a types file.
There was a problem hiding this comment.
Let's keep it consistent with how other components are structured for now, I guess
mirka
left a comment
There was a problem hiding this comment.
Looking forward to seeing this used in the app!
|
Thank you all 🙌 |
What?
Closes #74122
This adds a new
Skeletoncomponent to@wordpress/ui. It is a simple placeholder that you show while content is loading.It uses the design tokens, so it adapts to the theme. You can set
width,heightandradiuswith props (radius maps to the border radius tokens, orfullfor circles/pills). For now there is only one animation,pulse.As discussed in the issue, for the animation I based it on what shadcn does.
Why?
We have quite a few places in the editor where content loads in a bit abruptly. These transitions make the editor feel less polished, especially on slower connections. You can read more details on the why here.
How?
Added
Skeletoninpackages/ui/src/skeleton, following the same pattern as the other components (forwardRef,useRender, CSS Modules under thewp-uilayer). Used the design tokens, respecting reduced motion preferences and making the element hidden by default since it's decorative.Testing Instructions
npm run storybookScreenshots or screencast
skeleton-storybook.mp4