feat(animations): tasteful motion refresh with reduced-motion support#3375
Open
orrgottlieb wants to merge 4 commits into
Open
feat(animations): tasteful motion refresh with reduced-motion support#3375orrgottlieb wants to merge 4 commits into
orrgottlieb wants to merge 4 commits into
Conversation
…n tokens and reduced-motion support Introduces a motion-safe/motion-reduce mixin foundation in @vibe/style and applies it consistently across Toast, Modal, Skeleton, Tab, and the shared DialogContent primitive (which powers Tooltip, Menu submenus, and Dropdown). Replaces hardcoded durations and easing with motion tokens, normalizes keyframes to clean from/to pairs, and tightens scale-pop ratios for a more refined feel. All non-essential animations now respect prefers-reduced-motion. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Contributor
Code Review by Qodo
1. Submenu enter flash risk
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
Comment on lines
+1
to
+2
| @import "~@vibe/style/dist/mixins"; | ||
| @import "~@vibe/style/dist/mixins/motion"; |
Contributor
There was a problem hiding this comment.
1. Scss imports added in modal.module.scss 📘 Rule violation ⚙ Maintainability
Multiple CSS Module stylesheets (Modal.module.scss, Skeleton.module.scss, Toast.module.scss, DialogContent.module.scss, and Tab.module.scss) introduce new SCSS @import statements even though .module.scss files must not contain imports. This violates the repository’s CSS Modules styling compliance constraints and can undermine encapsulation guarantees.
Agent Prompt
## Issue description
Several `.module.scss` (CSS Modules) files add SCSS `@import` statements, but repository Rule 6 disallows any imports in `.module.scss` files.
## Issue Context
The added imports appear to be used to access motion-related mixins/helpers (e.g., `motion-safe` / `motion-reduce`, reduced-motion gating, and conditional transitions). To comply, remove per-file SCSS imports and replace mixin usage with direct media-query wrappers such as `@media (prefers-reduced-motion: no-preference)` and `@media (prefers-reduced-motion: reduce)`, or use another no-import compliant mechanism (e.g., a build-time injection approach) that does not require imports inside CSS Modules.
## Fix Focus Areas
- packages/core/src/components/Modal/Modal/Modal.module.scss[1-2]
- packages/core/src/components/Skeleton/Skeleton.module.scss[1-2]
- packages/core/src/components/Toast/Toast.module.scss[1-2]
- packages/components/dialog/src/Dialog/components/DialogContent/DialogContent.module.scss[2-2]
- packages/core/src/components/Tabs/Tab/Tab.module.scss[2-2]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
166
to
185
| .containerExitActive { | ||
| .overlay { | ||
| opacity: 0; | ||
| transition: opacity 100ms cubic-bezier(0.6, 0, 1, 1); | ||
| } | ||
| @include motion-safe { | ||
| .overlay { | ||
| opacity: 0; | ||
| transition: opacity var(--motion-productive-medium) var(--motion-timing-exit); | ||
| } | ||
|
|
||
| .centerPop { | ||
| animation: centerPopOut 100ms cubic-bezier(0.6, 0, 1, 1) forwards; | ||
| } | ||
| .centerPop { | ||
| animation: centerPopOut var(--motion-productive-long) var(--motion-timing-exit) forwards; | ||
| } | ||
|
|
||
| .anchorPop { | ||
| animation: anchorPopOut 150ms cubic-bezier(0.6, 0, 1, 1) forwards; | ||
| } | ||
| .anchorPop { | ||
| animation: anchorPopOut var(--motion-productive-long) var(--motion-timing-exit) forwards; | ||
| } | ||
|
|
||
| .fullView { | ||
| animation: fullViewOut 100ms cubic-bezier(0.6, 0, 1, 1) forwards; | ||
| .fullView { | ||
| animation: fullViewOut var(--motion-productive-medium) var(--motion-timing-exit) forwards; | ||
| } | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
2. Reduced-motion modal lingers 🐞 Bug ≡ Correctness
Modal.module.scss gates all exit styles under motion-safe, but Modal.tsx still uses a 150ms CSSTransition exit timeout. With prefers-reduced-motion: reduce, the modal stays visible and focus-locked for the full 150ms because no reduced-motion exit styles run and unmount is delayed.
Agent Prompt
### Issue description
`Modal.module.scss` wraps `.containerExitActive` entirely in `@include motion-safe`, so reduced-motion users get no exit animation styles. However, `Modal.tsx` still delays unmount by `timeout.exit` (150ms), causing the modal + focus lock to remain active/visible briefly after close.
### Issue Context
`CSSTransition` with `unmountOnExit` will keep children mounted until the timeout elapses, regardless of whether CSS animations are present.
### Fix Focus Areas
- packages/core/src/components/Modal/Modal/Modal.tsx[138-149]
- packages/core/src/components/Modal/Modal/Modal.module.scss[119-185]
### Suggested fix
1. Detect reduced motion in `Modal.tsx` (e.g. `window.matchMedia('(prefers-reduced-motion: reduce)')` or a small hook).
2. When reduced motion is enabled, pass `timeout={{ enter: 0, exit: 0 }}` (or `timeout={0}`) to `CSSTransition` so the modal unmounts immediately.
3. (Optional defense-in-depth) Add `@include motion-reduce` styles to `.containerExitActive` to hide/passthrough pointer events in case a non-zero timeout is ever used again.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
104
to
112
| .exitActive { | ||
| animation-iteration-count: 1; | ||
| animation-fill-mode: forwards; | ||
| animation-name: slideOut; | ||
| animation-duration: 350ms; | ||
| animation-timing-function: cubic-bezier(0.6, 0, 0.4, 1); | ||
| @include motion-safe { | ||
| animation: slideOut var(--motion-productive-long) var(--motion-timing-exit) forwards; | ||
| } | ||
|
|
||
| @include motion-reduce { | ||
| opacity: 0; | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
3. Invisible toast intercepts clicks 🐞 Bug ≡ Correctness
Toast.module.scss sets reduced-motion exit to opacity: 0 only, while Toast.tsx keeps CSSTransition
timeout={400} with unmountOnExit. This leaves an invisible, fixed-position toast mounted and
still pointer-interactive for up to 400ms, potentially blocking clicks in that area.
Agent Prompt
### Issue description
In reduced motion, `.exitActive` only sets `opacity: 0`, but the toast remains mounted for the full CSSTransition timeout (400ms). Because the element is `position: fixed` and pointer events are not disabled, it can intercept clicks while invisible.
### Issue Context
`CSSTransition` with `unmountOnExit` keeps the DOM node around until the timeout elapses.
### Fix Focus Areas
- packages/core/src/components/Toast/Toast.module.scss[98-112]
- packages/core/src/components/Toast/Toast.tsx[179-186]
### Suggested fix
- In `Toast.module.scss` under `@include motion-reduce` for `.exitActive`, add `pointer-events: none` (and optionally `visibility: hidden` or a transform that moves it out of the way) so it can’t block interaction while waiting to unmount.
- (Optional) If you want truly instant removal for reduced-motion users, conditionally set `CSSTransition` `timeout={0}` when `prefers-reduced-motion: reduce` (ensure this won’t break SSR expectations for Toast).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Contributor
|
📦 Bundle Size Analysis ✅ No bundle size changes detected. Unchanged Components
📊 Summary:
|
MenuItemSubMenu rendered DialogContentContainer directly with no transition wrapper, so submenus appeared and disappeared instantly. Wrap the submenu in CSSTransition with a placement-aware fade + scale-from-origin animation that mirrors Tooltip's expand variant — keyed off useFloating's resolved placement so the submenu grows out of the parent item's edge. Respects prefers-reduced-motion via the shared motion-safe mixin. Replaces the visibility-toggle render strategy with mount/unmount; updates the corresponding test to assert presence rather than visibility style. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Contributor
|
Persistent review updated to latest commit 8e58e40 |
Comment on lines
89
to
91
| .opacitySlideAppearActive { | ||
| transition: opacity 0.2s ease, transform 0.2s ease-out; | ||
| opacity: 1; | ||
| pointer-events: none; | ||
|
|
Contributor
There was a problem hiding this comment.
1. Reduced-motion blocks clicks 🐞 Bug ≡ Correctness
In DialogContent animations, pointer-events: none is applied outside motion-safe, so with prefers-reduced-motion: reduce the popover can mount fully visible but remain non-interactive for the full CSSTransition timeout. This is user-visible on click-trigger dialogs that rely on the default showDelay timeout (e.g. Info).
Agent Prompt
### Issue description
`pointer-events: none` is currently applied unconditionally on `*.AppearActive` classes. When `prefers-reduced-motion: reduce` is set, the `motion-safe` blocks don’t apply (no transition/opacity/transform rules), but the CSSTransition classes still apply and keep the content non-interactive for the whole timeout.
### Issue Context
- `DialogContent.tsx` always runs `CSSTransition` with `timeout={showDelay}`.
- Reduced-motion users should get the popover “at rest immediately”, which also implies it should be interactive immediately.
### Fix Focus Areas
- packages/components/dialog/src/Dialog/components/DialogContent/DialogContent.module.scss[89-106]
- packages/components/dialog/src/Dialog/components/DialogContent/DialogContent.module.scss[177-200]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+111
to
+114
| @include motion-safe { | ||
| transition: transform var(--motion-productive-long) var(--motion-timing-enter), | ||
| opacity var(--motion-productive-long) var(--motion-timing-enter); | ||
| opacity: 0; |
Contributor
There was a problem hiding this comment.
2. Dialogcontent timeout mismatch 🐞 Bug ≡ Correctness
DialogContent’s CSS now uses --motion-productive-long (150ms) for expand transitions, but
DialogContent.tsx still ends the transition after timeout={showDelay} (default 100ms). This
removes the active transition classes early, truncating the animation and making the new token
timing not actually take effect.
Agent Prompt
### Issue description
The JS-driven transition lifecycle duration (CSSTransition `timeout`) is shorter than the new CSS transition duration (150ms). This causes class removal before the CSS transition completes, effectively cutting animations short.
### Issue Context
- `Dialog.tsx` already uses `showDelay` to delay opening; `showDelay` should not also be used as the animation duration.
- Motion tokens define `--motion-productive-long: 150ms`.
### Fix Focus Areas
- packages/components/dialog/src/Dialog/components/DialogContent/DialogContent.tsx[185-233]
- packages/components/dialog/src/Dialog/Dialog.tsx[33-45]
- packages/components/dialog/src/Dialog/components/DialogContent/DialogContent.module.scss[109-114]
- packages/style/src/motion.scss[1-6]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Submenus only animated on close — open was instant. CSSTransition with mountOnEnter swaps the start/active classes in quick succession on a freshly-mounted node, and the browser can land both in the same paint cycle, skipping the "from" state entirely. Replace the transition-based approach with CSS keyframe animations on the active classes. Animations fire reliably on mount regardless of frame timing. Placement classes are now applied statically to the wrapper so the transform-origin is set before the animation runs. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Contributor
|
Persistent review updated to latest commit ab961eb |
| })} | ||
| </DialogContentContainer> | ||
| )} | ||
| <div style={floatingStyles} ref={refs.setFloating} data-popper-placement={actualPlacement}> |
Contributor
There was a problem hiding this comment.
1. menuitemsubmenu root lacks data-vibe 📘 Rule violation ◔ Observability
The updated root element for MenuItemSubMenu does not include the required [data-vibe] attribute. This breaks the standard component identification/instrumentation requirement.
Agent Prompt
## Issue description
`MenuItemSubMenu`'s root rendered element is missing the required `[data-vibe]` attribute.
## Issue Context
Compliance requires all React component roots to include `[data-vibe]` for consistent identification/instrumentation.
## Fix Focus Areas
- packages/core/src/components/Menu/MenuItem/components/MenuItemSubMenu/MenuItemSubMenu.tsx[58-60]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
The submenu now animates closed via CSSTransition with unmountOnExit, so the element stays mounted for ~100ms during the exit animation. Wrap the post-close `not.toBeInTheDocument()` assertions in `waitFor` so they await unmount instead of asserting synchronously. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Contributor
|
Persistent review updated to latest commit 97ae8b0 |
Comment on lines
+60
to
+71
| <CSSTransition | ||
| in={open} | ||
| appear | ||
| mountOnEnter | ||
| unmountOnExit | ||
| nodeRef={transitionRef} | ||
| timeout={{ appear: 150, enter: 150, exit: 100 }} | ||
| classNames={{ | ||
| appearActive: styles.appearActive, | ||
| enterActive: styles.enterActive, | ||
| exitActive: styles.exitActive | ||
| }} |
Contributor
There was a problem hiding this comment.
1. Submenu enter flash risk 🐞 Bug ≡ Correctness
MenuItemSubMenu’s CSSTransition only provides *Active classNames (no appear/enter base classes), so the submenu can mount at its resting (visible) styles for a paint and then snap to the keyframe from state when enterActive is applied, producing a visible flash on open. DialogContent avoids this by mapping an explicit appear class that sets the initial hidden/scale state before the active transition begins.
Agent Prompt
### Issue description
`MenuItemSubMenu` wires `CSSTransition` with only `appearActive`/`enterActive`/`exitActive`. Because there is no `appear`/`enter` base class that sets the initial hidden/scale state, the submenu element can render once at its default (opacity 1 / scale 1) before the active class is applied, then immediately jump to the keyframe `from` state (opacity 0 / scale 0.92), causing a flash.
### Issue Context
This code intentionally uses keyframes to avoid missing the “from” frame on mount, but it still needs an initial base class (like DialogContent’s `expandAppear`) to ensure the element is already in the correct starting state before the `*Active` class is applied.
### Fix Focus Areas
- packages/core/src/components/Menu/MenuItem/components/MenuItemSubMenu/MenuItemSubMenu.tsx[58-72]
- packages/core/src/components/Menu/MenuItem/components/MenuItemSubMenu/MenuItemSubMenu.module.scss[50-61]
### Suggested fix
1. Add base class mappings in `CSSTransition.classNames`, e.g. `appear: styles.appear`, `enter: styles.enter` (and keep existing `appearActive`/`enterActive`/`exitActive`).
2. In the SCSS module, add `.appear` and `.enter` rules (under `@include motion-safe`) that set the initial state to match your keyframe `from` values (e.g. `opacity: 0; transform: scale(0.92);`).
3. Keep the placement transform-origin classes as-is (they only set `transform-origin`).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
User description
Summary
Refreshes Vibe's animation language so popovers, Toast, Modal, Skeleton, and Menu submenus feel intentional and consistent — and respect
prefers-reduced-motionacross the board.motion-safe/motion-reducemixins in@vibe/stylegive every component a single, dependency-free way to gate non-essential motion.--motion-productive-*,--motion-expressive-*,--motion-timing-*) — no more hardcoded0.2s ease.0.8→0.92–0.94) for a more refined entrance.from/topairs; the natural overshoot now comes from--motion-timing-emphasizeinstead of bespoke0%/50%/100%curves.What changed by component
expandvariant now combines fade + scale-from-origin (was scale-only). Bothopacity-and-slideandexpandpaths gated onmotion-safe. Tightened scale to0.92.centerPop,anchorPop,fullViewenter/exit gated onmotion-safe. Token-based durations. Scale tightened to0.94.motion-safe, with explicitmotion-reducefallbacks (instant opacity). Slide usesmotion-timing-emphasizefor natural overshoot.motion-safe; reduce-motion users get a stableopacity: 0.7instead of a pulse.motion-safe.CSSTransitionwith placement-awaresubmenuExpandIn/submenuExpandOutkeyframes. Uses CSS animations rather than transitions because CSSTransition +mountOnEntercan collapse the start/active class swap into one paint and skip the "from" frame on a fresh mount. Visibility-toggle render strategy replaced with mount/unmount.Reduced-motion: the new headline capability
Before this branch, prod ignored
prefers-reduced-motion. Now Toast, Skeleton, Tooltip, Menu submenu, Modal, Tabs, and Dropdown all respect it.Note on the import pattern
A second import
@import "~@vibe/style/dist/mixins/motion";is added alongside the existing@import "~@vibe/style/dist/mixins";in each consumer. This is intentional:rollup-plugin-postcss's Sass importer prefers a partial-prefixed lookup before directory resolution, and in worktree-based dev setups node-resolve can walk up past the worktree boundary into a stale_mixins.scssin the main repo'snode_modules. Importing the partial directly bypasses that ambiguity.Test plan
yarn workspace @vibe/core build— cleanyarn workspace @vibe/dialog build— cleanyarn workspace @vibe/core test— 1338 tests passingyarn workspace @vibe/dialog test— 45 tests passingyarn workspace @vibe/core stylelint— cleanyarn workspace @vibe/dialog stylelint— cleanprefers-reduced-motion: reducein DevTools and confirm animations are suppressed (Toast/Skeleton get explicit fallbacks; others mount at rest)🤖 Generated with Claude Code
PR Type
Enhancement
Description
Implement motion-safe/motion-reduce mixins for reduced-motion support
Replace hardcoded animation durations with motion tokens across components
Tighten scale-pop ratios (0.8 → 0.92–0.94) for refined entrance animations
Normalize keyframes to clean from/to pairs with token-based timing
Animate Menu submenus with placement-aware fade + scale transitions
Update interaction tests to await animation completion with waitFor
Diagram Walkthrough
File Walkthrough
9 files
New motion-safe and motion-reduce mixinsExport motion mixins from main indexGate animations on motion-safe, use motion tokensApply motion tokens and reduce-motion support to modalsReplace hardcoded durations with motion tokensGate shine animation on motion-safe with fallbackGate tab indicator transition on motion-safeNew submenu animation styles with placement-aware transformsWrap submenu in CSSTransition with placement-aware animations2 files
Update tests to assert mount/unmount instead of visibilityAdd waitFor to await animation completion in tests