Skip to content
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

[charts] Ensure reduce motion preference disables animation on page load #14417

Merged
merged 16 commits into from
Oct 14, 2024

Conversation

JCQuintas
Copy link
Member

@JCQuintas JCQuintas commented Aug 30, 2024

Fixes #13477

Could be a starting point for #13964

  • Created a AnimationProvider which currently only holds the skipAnimation value
    • Migrated away from the useReduceMotion hook, as it got merged into the provider above
    • An alternative would have been to run the hook on every component that previously accepted skipAnimation, but that felt unnecessary, and the chosen solution could path a way to customising animations

By moving it to the provider, and passing it as a prop to all animations, we ensure immediate is called with the correct prop. I've left Globals.assign in place just in case, but it shouldn't be necessary anymore.

Testing

Emulating reduced motion in chrome

https://developer.chrome.com/docs/devtools/rendering/emulate-css#emulate_css_media_feature_prefers-reduced-motion

Running locally

pnpm --filter docs build && pnpm --filter docs serve

@JCQuintas JCQuintas added bug 🐛 Something doesn't work component: charts This is the name of the generic UI component, not the React module! labels Aug 30, 2024
@JCQuintas JCQuintas self-assigned this Aug 30, 2024
@mui-bot
Copy link

mui-bot commented Aug 30, 2024

Deploy preview: https://deploy-preview-14417--material-ui-x.netlify.app/

Generated by 🚫 dangerJS against 9637c1c

Copy link

codspeed-hq bot commented Aug 30, 2024

CodSpeed Performance Report

Merging #14417 will not alter performance

Comparing JCQuintas:skip-animation-on-page-load (9637c1c) with master (c2cff49)

Summary

✅ 3 untouched benchmarks

@JCQuintas
Copy link
Member Author

Failing tests are unrelated #14457

export type AnimationProviderProps = {
/**
* If `true`, animations are skipped.
* @default undefined
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a big fan of @default undefined because all props are by default undefined

May be more interesting to describe what happends by default

If unset, charts addapt the user's prefers-reduced-motion preferences

By the way since false does not allow to overrid the user preferences (which I think is expected) why not setting false as the default value?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adjusted it to:

If unset or false, the animations respects the user's prefers-reduced-motion setting.

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Sep 11, 2024
Copy link

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Oct 7, 2024
@JCQuintas JCQuintas merged commit fe69246 into mui:master Oct 14, 2024
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work component: charts This is the name of the generic UI component, not the React module!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[charts] browser reduce motion does not disable the initial animation in production
3 participants