-
Notifications
You must be signed in to change notification settings - Fork 43
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Introduce tokens to support loading patterns #744
Conversation
🦋 Changeset detectedLatest commit: 73f8697 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Variables changedNo variables changed |
Design Token Diff
|
@@ -1933,6 +1933,12 @@ | |||
}, | |||
}, | |||
}, | |||
skeletonLoader: { | |||
bgColor: { | |||
$value: '{bgColor.muted}', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to add an entry for dark mode if it's using the same token?
We use the same color between light and dark. We rely on bgColor.muted
changing between dark and light modes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we need dark and light mode, just not any of the sub-themes.
@@ -1931,6 +1931,12 @@ | |||
}, | |||
}, | |||
}, | |||
skeletonLoader: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't add color tokens for the spinner because we just use currentColor
with opacity. In most cases, this will mean the "track" of the spinner is fgColor.default
@ 25% opacity, the bit that spins around the track is fgColor.default
@ 100% opacity.
{ | ||
motion: { | ||
spinner: { | ||
rotationDuration: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure, but WDYT think about a more general approach like spinner.speed
and skeletonLoader.speed
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like duration
is different from speed
in animation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Than maybe using just duration
instead of rotationDuration
and shimmerDuration
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think duration
could work for this case. When we start to add tokens for more complex animations, we might need to specify what the duration is for.
For example: If I have an animation that scales in (transform: scale()
) at a different rate than it fades in (opacity
), we'll need to differentiate the two durations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, true. But we could also think about nested naming? E.g. maybe this is part of the "scale"?
duration.scale
and duration.opacity
?
Do you think we need to figure this out now, or should we go with duration
and see if we ever run into issues?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So motion-spinner-rotationDuration-default
could be motion-spinner-duration-rotation
with future tokens like..
motion-spinner-duration-scale
or motion-spinner-duration-opacity
I like this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One comment / idea about a more general approach for speed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! I would add a changeset for this with a patch, and then hopefully we'll resolve the build error so you can merge 😄
I updated this PR to main and now it has the duration validation support from #749 and the test is not failing anymore. |
I think we should add a transform for duration tokens that strips the unit. So CSS would get What do you think? |
We currently don't build |
Okay, I just merged the transformer as well: #751 |
@mperrotti once you add the changeset it should be ready to merge. The |
We might be able to get around using Also, we still style in JS in PRC (for now). |
This is changing with v8, right @langermank? That was why we removed js from the output? |
…tives into mp/loading-indicator-tokens
@mperrotti @lukasoppermann this is still an open question and I'm writing up a discussion/ADR to pave a path forward (using JS vs. CSS vars) |
Okay, whatever we decide will be fine. Adding js back in is relatively quick. |
Summary
The primary goal of this PR was to add a design token to represent the amount of time we should wait before rendering a loading indicator or announcing a loading state to assistive technologies.
I also took some time to add other tokens I know we'll need to support the loading spinner and the "skeleton" loader.
List of notable changes:
Added
What should reviewers focus on?
Steps to test:
TBD
Supporting resources (related issues, external links, etc):
Contributor checklist:
Reviewer checklist: