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.

Sign up for GitHub

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

Jump to bottom

[charts] Fix LineChart transition stopping before completion #14366

Merged
merged 3 commits into from
Aug 28, 2024
Merged

[charts] Fix LineChart transition stopping before completion #14366

merged 3 commits into from
Aug 28, 2024

Conversation

JCQuintas
Copy link
Member

@JCQuintas JCQuintas commented Aug 27, 2024 •
edited
Loading

  • 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 •
edited
Loading

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

Generated by 🚫 dangerJS against 4c23244

@codspeed-hq CodSpeed HQ
Copy link

codspeed-hq bot commented Aug 27, 2024 •
edited
Loading

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

alexfauquette
alexfauquette reviewed Aug 27, 2024
View reviewed changes
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

packages/x-charts/src/internals/useAnimatedPath.ts Outdated
@@ -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

Janpot
Janpot approved these changes Aug 28, 2024
View reviewed changes
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.

👍 Verified that it fixes #12873
https://codesandbox.io/p/sandbox/exciting-wright-m4s345?file=%2Fpackage.json%3A6%2C82

alexfauquette
alexfauquette approved these changes Aug 28, 2024
View reviewed changes
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
@JCQuintas JCQuintas mentioned this pull request Aug 28, 2024
Merged
@mbreadmachine mbreadmachine mentioned this pull request Sep 20, 2024
Closed
This was referenced Sep 20, 2024
Open
Open
Open
Open
Open
@mbreadmachine mbreadmachine mentioned this pull request Sep 25, 2024
Closed
This was referenced Sep 27, 2024
Open
Open
Open
Open
This was referenced Oct 3, 2024
Open
Open
@mbreadmachine mbreadmachine mentioned this pull request Oct 5, 2024
Closed
This was referenced Oct 5, 2024
Open
Open
Open
@mbreadmachine mbreadmachine mentioned this pull request Oct 6, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alexfauquette alexfauquette alexfauquette approved these changes

@Janpot Janpot Janpot approved these changes

Assignees

@JCQuintas JCQuintas

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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[charts] LineChart transition stops before completion
4 participants
@JCQuintas @mui-bot @Janpot @alexfauquette