-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Add reduceMotion option to animate() and make useAnimate respect MotionConfig #3500
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
Conversation
Code Review: Add reduceMotion option to animate() and make useAnimate respect MotionConfigSummaryThis PR successfully addresses issues #2593 and #2771 by adding explicit ✅ Strengths
|
b587354 to
39a1b60
Compare
PR Review - Add reduceMotion option to animate() and make useAnimate respect MotionConfigI've reviewed this PR and here's my feedback: ✅ Positives
🔍 Areas for Improvement1. Potential Issue with useAnimate Option Merging (packages/framer-motion/src/animation/hooks/use-animate.ts:28-35)The current implementation may not correctly handle cases where users explicitly pass const mergedOptions = {
reduceMotion: shouldReduceMotion, // This comes first
...options, // User options spread after
}Problem: If a user explicitly passes Suggestion: Consider adding a comment explaining the precedence, or explicitly check for undefined: const mergedOptions = {
...options,
reduceMotion: options?.reduceMotion ?? shouldReduceMotion,
}2. Missing Test CoverageThe PR checklist shows unchecked test items. For a feature this important, comprehensive tests are critical:
3. Sequence Animation Type Safety (packages/framer-motion/src/animation/hooks/use-animate.ts:15-24)The sequence detection uses return ((
subjectOrSequence: any,
optionsOrKeyframes?: any,
options?: any
) => {Suggestion: Consider using proper type guards and conditional types to maintain type safety, or at minimum add a comment explaining why 4. Documentation of Sequence Option Spreading (packages/framer-motion/src/animation/sequence/create.ts:388)In 🎯 Performance ConsiderationsThe use of 🔒 Security ConsiderationsNo security concerns identified. The changes are purely behavioral and don't introduce any new attack vectors. 📝 Code StyleThe code follows the repository conventions from CLAUDE.md:
🎨 Suggestions
Overall AssessmentThis is a well-designed solution that addresses a real pain point in the library. The implementation is clean and follows established patterns. The main concern is the lack of test coverage for the new functionality. Once tests are added and the minor issues above are addressed, this will be ready to merge. Recommendation: Address test coverage before merging, consider the option merging precedence comment for clarity. |
39a1b60 to
f1d9714
Compare
PR Review: Add reduceMotion option to animate() and make useAnimate respect MotionConfigSummaryThis PR addresses issues #2593 and #2771 by adding explicit ✅ Strengths
|
…onConfig This fixes two related issues: - #2593: MotionConfig reducedMotion setting doesn't affect animate() function - #2771: animate() automatically disables transform/layout animations based on device preference without developer control Changes: - Add reduceMotion option to AnimationOptions allowing explicit override - useAnimate now automatically injects reduceMotion based on MotionConfig context - Pass reduceMotion through animateTarget to override visualElement.shouldReduceMotion Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
f1d9714 to
b32b466
Compare
Pull Request ReviewThank you for addressing issues #2593 and #2771! This is a well-structured implementation that gives developers explicit control over reduced motion behavior. Here's my detailed review: ✅ Strengths
🔍 Potential Issues & Questions1. Sequence Animation Option Propagation (packages/framer-motion/src/animation/sequence/create.ts)The Current flow (lines 42-44): export function createAnimationsFromSequence(
sequence: AnimationSequence,
{ defaultTransition = {}, ...sequenceTransition }: SequenceOptions = {},The definition.transition[key] = {
...remainingDefaultTransition,
duration: totalDuration,
ease: valueEasing,
times: valueOffset,
...sequenceTransition, // ← reduceMotion is here
}Then in animations.push(...animateSubject(subject, keyframes, transition))Question: Does the const transition = { ...options }This should work, but I'd verify that Test recommendation: Add a specific test for sequence animations with animate([
[el1, { x: 100 }],
[el2, { y: 100 }]
], { reduceMotion: false })2. useMemo Dependency Stability (packages/framer-motion/src/animation/hooks/use-animate.ts:16-21)const reduceMotion = useReducedMotionConfig() ?? undefined
const animate = useMemo(
() => createScopedAnimate({ scope, reduceMotion }),
[scope, reduceMotion]
)Potential issue: The Question: Should the animate function be recreated when
This is probably the desired behavior, but worth confirming. 3. Type Safety in animateTarget() (packages/motion-dom/src/animation/interfaces/visual-element-target.ts:42)const reduceMotion = (transition as { reduceMotion?: boolean })?.reduceMotionIssue: Using type assertion instead of proper typing. The Suggestion: Update the interface TargetAndTransition {
transition?: Transition & { reduceMotion?: boolean }
// ...
}Or add it to the 4. Missing Test CoverageThe PR includes a test plan in the description but no actual tests. Per CLAUDE.md:
Critical: Please add tests covering:
Consider Playwright/E2E tests for gesture handling as suggested in CLAUDE.md. 5. DocumentationJSDoc improvements needed: In interface ScopedAnimateOptions {
scope?: AnimationScope
/**
* Whether to reduce motion for transform/layout animations.
*
* - `true`: Skip transform/layout animations (instant transition)
* - `false`: Always animate transforms/layout
* - `undefined`: Respect device preference (default)
*
* @default undefined
*/
reduceMotion?: boolean
}The JSDoc in motion-dom/src/animation/types.ts:595-604 is good, but consider adding examples: /**
* @example
* // Override device preference
* animate(el, { x: 100 }, { reduceMotion: false })
*
* @example
* // Skip transform animations
* animate(el, { x: 100 }, { reduceMotion: true })
*/🎯 Edge Cases to Consider
🔒 Security & Performance
📋 RecommendationsBefore merge:
Nice to have: 🎉 ConclusionThis is a solid implementation that solves real user pain points. The architecture is clean and the feature integrates well with the existing codebase. Once tests are added and the type safety issue is addressed, this will be ready to merge. Estimated effort to address blockers: 1-2 hours for tests + 15 minutes for type fix. Great work on addressing these long-standing issues! 🚀 |
Summary
reduceMotionoption toanimate()allowing developers to explicitly control transform/layout animation behavioruseAnimateautomatically respectMotionConfig reducedMotionsettinganimate()ignored MotionConfig and provided no way to override device preferenceFixes #2593, fixes #2771
Test plan
animate(el, { x: 100 }, { reduceMotion: false })animates transforms even with device reduced motion enabledanimate(el, { x: 100 }, { reduceMotion: true })skips transform animationsuseAnimatewithin<MotionConfig reducedMotion="never">animates transformsuseAnimatewithin<MotionConfig reducedMotion="always">skips transform animationsreduceMotionoption🤖 Generated with Claude Code