-
Notifications
You must be signed in to change notification settings - Fork 13
fix: bring component motion in line with animation standards #852
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -40,7 +40,7 @@ | |||||||||||||||||||||||||||||||||||||||||
| letter-spacing: var(--rs-letter-spacing-small); | ||||||||||||||||||||||||||||||||||||||||||
| outline: none; | ||||||||||||||||||||||||||||||||||||||||||
| box-sizing: border-box; | ||||||||||||||||||||||||||||||||||||||||||
| transition: all 0.2s ease; | ||||||||||||||||||||||||||||||||||||||||||
| transition: var(--rs-transition-interactive); | ||||||||||||||||||||||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win Gate the The transition is declared directly in ♿ Proposed fix to wrap transition in reduced-motion media query .input {
width: 100%;
padding: 0 var(--rs-space-3);
border-radius: var(--rs-radius-2);
border: 1px solid var(--rs-color-border-base-tertiary);
background: var(--rs-color-background-base-primary);
font-weight: var(--rs-font-weight-regular);
font-size: var(--rs-font-size-small);
line-height: var(--rs-line-height-small);
letter-spacing: var(--rs-letter-spacing-small);
outline: none;
box-sizing: border-box;
- transition: var(--rs-transition-interactive);
height: var(--rs-space-9);
}
+
+@media (prefers-reduced-motion: no-preference) {
+ .input {
+ transition: var(--rs-transition-interactive);
+ }
+}📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||||||
| height: var(--rs-space-9); | ||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -86,11 +86,11 @@ | |
|
|
||
| @media (prefers-reduced-motion: no-preference) { | ||
| .indicator { | ||
| transition: width 500ms; | ||
| transition: width var(--rs-duration-moderate) linear; | ||
| } | ||
|
|
||
| .circularIndicatorCircle { | ||
| transition: stroke-dashoffset 500ms; | ||
| transition: stroke-dashoffset var(--rs-duration-moderate) linear; | ||
|
Comment on lines
+89
to
+93
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift Verify the 500ms → 250ms duration reduction is intentional. The previous hardcoded 🤖 Prompt for AI Agents |
||
| } | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -86,11 +86,11 @@ | |
|
|
||
| @media (prefers-reduced-motion: no-preference) { | ||
| .indicator { | ||
| transition: width 500ms; | ||
| transition: width var(--rs-duration-moderate) linear; | ||
| } | ||
|
|
||
| .circularIndicatorCircle { | ||
| transition: stroke-dashoffset 500ms; | ||
| transition: stroke-dashoffset var(--rs-duration-moderate) linear; | ||
|
Comment on lines
+89
to
+93
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift Same 500ms → 250ms duration reduction as meter — verify intentional. This file mirrors the meter changes exactly. The same concern about halving the animation duration applies here. Keeping meter and progress on the same token is good for consistency, but confirm the faster fill rate is the desired UX. 🤖 Prompt for AI Agents |
||
| } | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -20,6 +20,7 @@ | |||||||||
| justify-content: center; | ||||||||||
| cursor: pointer; | ||||||||||
| box-sizing: border-box; | ||||||||||
| transition: background-color 150ms ease, border-color 150ms ease; | ||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win Use shared motion tokens instead of hardcoded values. Same issue as ♻️ Proposed fix .radioitem {
/* ... */
- transition: background-color 150ms ease, border-color 150ms ease;
+ transition:
+ background-color var(--rs-duration-fast) var(--rs-ease-out),
+ border-color var(--rs-duration-fast) var(--rs-ease-out);
}📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||
| } | ||||||||||
|
|
||||||||||
| /* Group-level size — low specificity via :where() so item-level size wins */ | ||||||||||
|
|
@@ -74,6 +75,26 @@ | |||||||||
| width: 100%; | ||||||||||
| height: 100%; | ||||||||||
| position: relative; | ||||||||||
| transition: opacity var(--rs-duration-fast) var(--rs-ease-out); | ||||||||||
| } | ||||||||||
|
|
||||||||||
| .indicator[data-starting-style], | ||||||||||
| .indicator[data-ending-style], | ||||||||||
| .indicator[data-unchecked] { | ||||||||||
| opacity: 0; | ||||||||||
| } | ||||||||||
|
|
||||||||||
| @media (prefers-reduced-motion: no-preference) { | ||||||||||
| .indicator { | ||||||||||
| transition: | ||||||||||
| opacity var(--rs-duration-fast) var(--rs-ease-out), | ||||||||||
| transform var(--rs-duration-fast) var(--rs-ease-out); | ||||||||||
| } | ||||||||||
|
|
||||||||||
| .indicator[data-starting-style], | ||||||||||
| .indicator[data-ending-style] { | ||||||||||
| transform: scale(0.8); | ||||||||||
| } | ||||||||||
| } | ||||||||||
|
|
||||||||||
| .indicator::after { | ||||||||||
|
|
||||||||||
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.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Use shared motion tokens instead of hardcoded values.
The base
.checkboxtransition uses150ms easewhile the.indicator(line 111) correctly usesvar(--rs-duration-fast) var(--rs-ease-out). Theeasekeyword (cubic-bezier(0.25, 0.1, 0.25, 1)) is also a different curve from--rs-ease-out, so even if durations match, the easing will feel inconsistent between background/border and the indicator. This contradicts the PR objective of consolidating easing and duration into shared tokens.♻️ Proposed fix
📝 Committable suggestion
🤖 Prompt for AI Agents