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] Fix LineChart transition stopping before completion #14366

Conversation

JCQuintas
Copy link
Member

@JCQuintas JCQuintas commented Aug 27, 2024

  • Ensures we always have the previous and current values
  • Ensures the animation resets only when the current path value changes

Fixes #12873
Closes #12892

Fixes original issue: https://codesandbox.io/p/sandbox/solitary-cookies-2k447w?file=%2Fsrc%2FDemo.tsx%3A7%2C1&workspaceId=836800c1-9711-491c-a89b-3ac24cbd8cd8
And example issue opened on react-spring
https://stackblitz.com/edit/vitejs-vite-9sj21g?file=src%2FApp.tsx

@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 27, 2024
@JCQuintas JCQuintas self-assigned this Aug 27, 2024
@dosubot dosubot bot added the size:M This PR changes 30-99 lines, ignoring generated files. label Aug 27, 2024
@mui-bot
Copy link

mui-bot commented Aug 27, 2024

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

Generated by 🚫 dangerJS against 4c23244

Copy link

codspeed-hq bot commented Aug 27, 2024

CodSpeed Performance Report

Merging #14366 will not alter performance

Comparing JCQuintas:fix-LineChart-transition-stops-before-completion (4c23244) with master (0c30a6f)

Summary

✅ 3 untouched benchmarks

Copy link
Member

@alexfauquette alexfauquette left a comment

Choose a reason for hiding this comment

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

Looks nice, but it still causes issues when adding/removing a line to the chart

Capture.video.du.2024-08-27.18-07-00.mp4

https://deploy-preview-14366--material-ui-x.netlify.app/x/react-charts/lines/#animation

@@ -3,27 +3,39 @@ import { interpolateString } from '@mui/x-charts-vendor/d3-interpolate';
import { useSpring, to } from '@react-spring/web';

function usePrevious<T>(value: T) {
const ref = React.useRef<T | null>(null);
const ref = React.useRef<{ current: T; previous?: T }>({
current: value,
Copy link
Member

Choose a reason for hiding this comment

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

Maybe renaming it with actual or currentValue to avoid the confusion with the current of the ref

@JCQuintas
Copy link
Member Author

Looks nice, but it still causes issues when adding/removing a line to the chart

We were using useEffect wrong, which meant line animations were 1 step behind the current state 😢

@JCQuintas
Copy link
Member Author

These two seem to be unrelated though: #14355

Copy link
Member

@Janpot Janpot left a comment

Choose a reason for hiding this comment

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

Copy link
Member

@alexfauquette alexfauquette left a comment

Choose a reason for hiding this comment

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

Happy to see this fixed 🎉

I don't see any issue 👍

@JCQuintas JCQuintas merged commit 97afd65 into mui:master Aug 28, 2024
19 checks passed
@JCQuintas JCQuintas deleted the fix-LineChart-transition-stops-before-completion branch August 28, 2024 10:52
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! size:M This PR changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[charts] LineChart transition stops before completion
4 participants