Update the Spinner design #37551
Update the Spinner design #37551
Conversation
|
Size Change: +79 B (0%) Total Size: 1.14 MB
ℹ️ View Unchanged
|
amustaque97
added
[Package] Components
|
This is very nice! Thanks for working on it. I also really love these designs: Is there any way we can get closer to these in terms of design? Stroke-width, white instead of spot color in dark contexts, a gray base circle track? |
|
The only way I can think to include the track would be to use an svg rather than css alone, otherwise the mask is interfering too much. I've pushed an update to try this. spinner.mp4I'm not sure what the stroke width is in those screenshots, but I've set them to 1.5px for now. Easily changed. I can't think of a way to make the colors adapt based on the background as the Spinner isn't aware of that. This might be something better left as an option on the Spinner itself. |
There was a problem hiding this comment.
The SVG is working really well here. For testing it I managed to record this GIF:
I can't help but feel like the material influence here makes it a bit stuttery. The way the expansion and contraction of the blue indicator makes it appear as if it's pausing. I may be rather dull in the following, but a simple uniform rotation with no contracting might actually be preferable.
I'm going to tinker a bit in this codepen and come back! Thanks again for the work 👌
| @@ -0,0 +1,44 @@ | |||
| .components-spinner--svg { | |||
There was a problem hiding this comment.
The SVG is working fairly well here! However just as far as classnames go, should we call it "track" instead? Also, I think it should be __ instad of --. I.e. .components-spinner__track for example.
| left: 0; | ||
| } | ||
|
|
||
| @keyframes rotatesvg { |
There was a problem hiding this comment.
Same comment with above, denoting the element as "track" might be better.
It seems correct to me to put this keyframe statement right in this components file where it will be used, especially since it's unique to this component and not reused. But naming-wise, we could use some BEM niceness here as well, for example components-spinner__spin-track perhaps?
| } | ||
| } | ||
|
|
||
| .components-spinner--circle { |
There was a problem hiding this comment.
If I understand correctly, this is the blue part of the spinner that in this PR contracts and expands, so it's more of a quarter-circle. I realize this is nitpick territory, but I think it'd help readability. How about something like .components-spinner__indicator? Note the __ instead of --.
| transform-origin: 50% 50%; | ||
| } | ||
|
|
||
| @keyframes rotatecircle { |
There was a problem hiding this comment.
BEM all the way. How about components-spinner__spin-indicator?
|
Another reason to maybe reconsider the expansion and contraction of the indicator is that it isn't relative to the size of the spinner. While I don't know exactly how often we'll show a 128x128 spinner, a few things broke apart when I tried it:
It's actually still expanding and contracting, it's just not very visible because the dashoffset change is insignificant when large. I wonder if you could use Another one is the fact that the gray track is outset compared to the blue indicator. I don't know why that happens. Edit: Oh I know, the radius of the circle is 8.25, it should be 9 to match with half of the 18x18 svg viewbox. Combined with overflow: visible; on the SVG, it works. |
|
Using an SVG seems appropriate enough for this, but I'm starting to think that using We might need a static SVG that just draws that quarter-circle precisely, and not use the dasharray trick 🤔 Edit: Feel free to tinker in this codepen: https://codepen.io/joen/pen/RwLeByP?editors=1100 |
Perhaps we can add some logic so that the dasharray value(s) are based on the spinner size? I appreciate it's subjective/intangible, but I quite like the expansion/contraction effect. I can't put my finger on why but it makes it feel like something is happening compared with the linear spinner. Maybe it's the pulsing. Then again, if we eliminate that effect it would make the original design much easier to accomplish without the need for an svg. Maybe we can add a pulsing effect another way 🤔 I will also have a play in codepen :D |
|
I added a "Pulsey no svg" version to this codepen. Gif:
The main drawback to this approach is that it uses borders for the indicator line, which cannot be set to a 1.5px width. |
|
Impressive work going on in these codepens, I love it. I'm also starting to see your point about the contraction/expansions being a bit more dynamic. But just looking at them side by side, I'm personally still most attracted to the complete calmness of the linearly rotating circle, I feel like it might just work the best in noisy contexts. |
|
Reverted to html + css only, and simplified the animation. Here's the result:
The last question seems to be what to do about the discrepancy between track/indicator width. |
|
Clever trick to use border-top to grab exactly one quarter of the circle! I suppose box-shadow wouldn't work for that, as it would be affected by the contours of the circle shape. I do think it's important we get the right stroke width, though — maybe the SVG was fine after all? That might also let us keep that neat rounded edgecap. Alternatively we could create another full circle and mask a quadrant of it? |
|
To get the strokes to be 1.5px I think we'll have to return to the SVG approach. I just pushed a change that does this, and includes logic so that the dasharray will scale correctly. You can change the
|
|
I mean, I really dig that one 👌 Is this one ready for final review? |
|
Yup, I think so! |
There was a problem hiding this comment.
This is working really well for me:
It's a nice and simple spinner that benefits from the spot color.
I know you had your heart set on some expansion/contraction of the indicator, I think that's still something that can be explored in a codepen if you're finding yourself missing it. But because it's so rarely shown (thankfully things load fast), starting minimalist like this feels like a good baseline.
I left a few comments, but this looks good. Nice work, thank you!
There was a problem hiding this comment.
Hey @jameskoster , thank you for your work, and sorry for the late review — I wasn't away of this PR until @sarayourfriend pinged me.
I left a few comments around — would you also be able to add an entry to the CHANGELOG ? Thank you!
| const viewBox = `0 0 ${ CONFIG.spinnerSize } ${ CONFIG.spinnerSize }`; | ||
| const radius = Number( CONFIG.spinnerSize ) / 2; | ||
| const strokeDasharray = [ | ||
| Number( CONFIG.spinnerSize ), | ||
| Number( CONFIG.spinnerSize ) * 10, | ||
| ]; |
There was a problem hiding this comment.
Sorry if missed this while skimming through the previous conversations, but is there a reason why we compute dynamically viewBox, radius and strokeDasharray, all in relation to spinnerSize ?
With SVG we should be able to define an arbitrary viewBox and then scale everything accordingly. We could then use spinnerSize in the styles to change width and height of the SVG element itself (example)
There was a problem hiding this comment.
Yeah that would be a lot simpler wouldn't it :) We'll need to keep the computed stroke-dasharray though in order for the indicator to scale correctly, so we'll probably need to add a size property to the Spinner in the future.
| @@ -48,7 +48,7 @@ export default { | |||
| borderWidth: '1px', | |||
| borderWidthFocus: '1.5px', | |||
| borderWidthTab: '4px', | |||
| spinnerSize: '18px', | |||
| spinnerSize: Number( '18' ), | |||
There was a problem hiding this comment.
@ciampo let me know if this is correct.
There was a problem hiding this comment.
You should be able to just use the value 18, without wrapping it in Number(). You could then also update the stroke-dasharray attribute, since it doesn't require a runtime calc anymore
For example (click to expand):
diff --git a/packages/components/src/spinner/index.js b/packages/components/src/spinner/index.js
index b256e44a75..f30a852403 100644
--- a/packages/components/src/spinner/index.js
+++ b/packages/components/src/spinner/index.js
@@ -39,8 +39,7 @@ export const SpinnerIndicator = styled.circle`
stroke-linecap: round;
stroke-width: 1.5px;
transform-origin: 50% 50%;
- stroke-dasharray: ${ CONFIG.spinnerSize },
- calc( ${ CONFIG.spinnerSize } * 10 );
+ stroke-dasharray: ${ CONFIG.spinnerSize }, ${ CONFIG.spinnerSize * 10 };
`;
export default function Spinner() {
diff --git a/packages/components/src/utils/config-values.js b/packages/components/src/utils/config-values.js
index 526f85c457..7ba2b9d3dc 100644
--- a/packages/components/src/utils/config-values.js
+++ b/packages/components/src/utils/config-values.js
@@ -48,7 +48,7 @@ export default {
borderWidth: '1px',
borderWidthFocus: '1.5px',
borderWidthTab: '4px',
- spinnerSize: Number( '18' ),
+ spinnerSize: 18,
fontSize: '13px',
fontSizeH1: 'calc(2.44 * 13px)',
fontSizeH2: 'calc(1.95 * 13px)',There was a problem hiding this comment.
I get an error when I try it this way 🤔
Thinking about this a little more, do we actually need the spinnerSize config value? It doesn't seem to serve much of a purpose right now, especially as all Spinner instances are currently the same size. Might a better approach be to allow a size prop on the Spinner component? That way it's easy to use different sized spinners ad hoc.
There was a problem hiding this comment.
I get an error when I try it this way 🤔
It seems like your IDE thinks that spinnerSize is a string — did you also update spinnerSize in the config (as part of the diff above)? i.e
| spinnerSize: Number( '18' ), | |
| spinnerSize: 18, |
Thinking about this a little more, do we actually need the spinnerSize config value? It doesn't seem to serve much of a purpose right now, especially as all Spinner instances are currently the same size. Might a better approach be to allow a size prop on the Spinner component? That way it's easy to use different sized spinners ad hoc.
Let's leave it in the config for now, as it matches the $spinner-size variable in the base styles. We have plans later this year to take a closer look at the overall config variables :)
Re. adding a size prop, let's investigate that separately from this PR, maybe while we also address some of the ideas discussed in #37551 (comment)
There was a problem hiding this comment.
did you also update spinnerSize in the config (as part of the diff above)? i.e
Derp, I did not :)
Re. adding a size prop, let's investigate that separately from this PR
Okay! I think we'll need to do something, because atm if you use a Spinner and resize it with css alone it won't scale correctly:
There was a problem hiding this comment.
Oh nice, this looks more comprehensive – thanks so much :) Please feel free to push the changes.
Do you think it's worth including the preset sizes in the component as a part of this PR? If so the only tweak I would suggest would be to use multiples of our grid unit (or spinner-size) values for the medium/large spinners.
Now that I think of it, it might be a good idea to make the default spinnerSize 16px so that it better aligns with the broader grid.
There was a problem hiding this comment.
Please feel free to push the changes.
Thank you ! I went ahead and pushed those changes (including a change to the README to remove the out of date screenshot).
Do you think it's worth including the preset sizes in the component as a part of this PR? If so the only tweak I would suggest would be to use multiples of our grid unit (or spinner-size) values for the medium/large spinners.
I did include the updates to the Storybook examples, if anything to test that the stroke width is not changing as we increase the overall size of the spinner — I also made sure that those storybook examples use multiples of the grid unit.
For now, I wouldn't include any new APIs to the component (e.g. size), since the previous one also didn't have it.
Now that I think of it, it might be a good idea to make the default spinnerSize 16px so that it better aligns with the broader grid.
I haven't made that change yet as I'm not sure if resizing the spinner (from 18px to 16px) would have any ripercussions / would be considered breaking change. @jasmussen do you have any opinions on this?
There was a problem hiding this comment.
Thanks, this is looking fantastic :)
I think there's a reasonable case to resize from 18px to 16px. It aligns with our grid unit, and most other icons tend to render at ~16px, e.g. Styles icon:
Not a strong feeling though, and happy to defer to @jasmussen on this point.
There was a problem hiding this comment.
Done in 91a861f
Let me know once you've had a look, I think we may be able to merge this PR :)
|
Thanks @ciampo.
I thought this got updated automatically? 🤔 |
… the indicator (instead of stroke-dashoffset)
ciampo
force-pushed
the
update/spinner
branch
from
a3c980e to
2919d56
Compare
There was a problem hiding this comment.
would you mind rebasing on top of trunk to include those changes and solve conflicts? (Feel free to delete Riad's fix on this PR, as it was already included with the merge of #38237).
I took the freedom of doing that (hope you don't mind, @jameskoster !)
We can have a final look at the PR and hopefully everything will be in a good shape 🤞
I gave one final quick look and everything seems to work well — although I'd appreciate if other folks could also give it a final look before merging!
| display: block; | ||
| margin: auto; |
There was a problem hiding this comment.
As I gave one last look at the code changes, I just noticed a couple of differences that I'd like to discuss:
display: it used to beinline-block, it's currentlyblock. I personally likeblockmore, but this change could be potentially breaking for some layoutsmargin: it used to be5px 11px 0, it's notauto. This is also a potentially breaking change. I personally believe that a component should not define rules for its own margin, and so my preference would be to either keep things as they are, or to introduce a breaking change by removing themarginrule altogether.
What do folks think? (also cc @mirka @jasmussen )
There was a problem hiding this comment.
High level I do like the idea of unifying and systematizing how the spinner appears, for example always centered. But it also seems like something that needs a little exploration and guideline in terms of where the spinner is currently used, and might involve updating the places where it's used. That seems to suggest a separate exploratory PR for that, and then in order to land this big improvement, perhaps going back to inline-block. Though I do think we find a better margin, perhaps $grid-unit-10 $grid-unit-10 0?
There was a problem hiding this comment.
I agree with your point.
Probably best thing to do here is to switch back to the values we had before (display: inline-block and margin: 5px 11px 0), to make sure we don't introduce any breaking changes.
We can then definitely explore what to do separately, as you suggested
There was a problem hiding this comment.
I'm unaware of the history around the margin value, but the results are suboptimal in places like the Site Logo and Template Part blocks. margin: auto; works better as a default in that regard.
Then again the idea of having no margin on the Spinner at all does seem better on the surface. So I'm happy to leave the margin/display values as-is and revisit separately.
There was a problem hiding this comment.
Oh definitely we need that. Just in order to land this one with minimal disruption, we can pull back on those, and then follow up in small PRs, either a separate PR that just re-adds the auto margin entirely, or adds the margin in the contexts where they are needed.
There was a problem hiding this comment.
Cool, I'll go ahead, push these changes, give it a "final final" look and hopefully merge
There was a problem hiding this comment.
Merged!
follow up in small PRs, either a separate PR that just re-adds the auto margin entirely, or adds the margin in the contexts where they are needed.
My preference would be to remove the margin rule altogether from Spinner and add any necessary margin where each instance of Spinner is used
There was a problem hiding this comment.
Generally agree that the component itself should be unopinionated and subject to its container. But given very few existing containers are likely to be aware of this new responsibility, it might still make sense to have something. We could then consider that a default margin (either a fixed clearance all around, or auto margins), which would then be easily overriden by it's container?
There was a problem hiding this comment.
My preference would be to remove the
marginrule altogether fromSpinnerand add any necessarymarginwhere each instance ofSpinneris used
This is my general preference, too. And if the audit shows that it's frequently used as an inline item, I'd consider giving it some padding (instead of margin) so it isn't flush with the bounding box. Basically like an icon component.
|
Round of applause! It really needed an update 😄 |
|
Aside from the margin issue above, everything seems to be working well. Thanks for all the help :) |
…s version of the component
ciampo
force-pushed
the
update/spinner
branch
from
bf3d4b7 to
d06c635
Compare
|
@jameskoster First up - great work here. This is a big improvement! 🤝 👏 . I've been looking into improving the communication around loading states in a block I'm working on. I'm curious did you do any research into whether using I noticed you added Also did anyone from the Core Accessibility team review this PR? |
I think you are referring to a different Spinner :D In terms of this PR there was a little discussion around a11y, but the primary focus was to update the visuals. |
References: * WordPress/gutenberg#49695 * WordPress/gutenberg#37551 * p1683530650412459-slack-C0299DMPG
jameskoster
added
the
Design System
This PR updates the Spinner component based on the designs in #36725. It also adjusts/fixes the position of the Spinner that appears when searching for documents in the add-link interface. Finally, it adds the gray-300 value to color-values.js, which has until now been missing. On this point I did notice that gray-200 has the incorrect value, but as I'm not sure where that is used, I figured we should fix it separately.
Things seem to be working well in the following scenarios:
Testing instructions