-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
[charts] Fix LineChart
not properly animating when hydrating
#14355
[charts] Fix LineChart
not properly animating when hydrating
#14355
Conversation
Deploy preview: https://deploy-preview-14355--material-ui-x.netlify.app/ |
CodSpeed Performance ReportMerging #14355 will not alter performanceComparing Summary
|
…chart-client-side-render-issue
c2101de
to
6b8a457
Compare
…48c745322dbce917/src/index.ts#L7
It seems that the only way to test this is in a different repository, since our nextjs setup prevents the issues from appearing 😓 @alexfauquette do you think this requires a custom test to prevent regression? We could have a ci step that |
For sure it's to much work since we nearly never touch those animation |
pnpm-lock.yaml
Outdated
peerDependencies: | ||
'@types/react': ^17.0.0 || ^18.0.0 | ||
'@types/react': 18.3.3 | ||
react: ^17.0.0 || ^18.0.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Quite strange. Could it be a modification from another work that leaked in this PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Result of pnpm dedupe
🤔
happens on master as well
I'm not sure why the ci check pnpm dedupe --check
doesn't pick it up, it also doesn't pick up locally. I guess because it is a peer dep? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I ran dedupe to try to fix the netlify build, just in case 😓
#14366 introduced an issue on hydration that further made the current behaviour worse for hydrated line charts 😓 The current solution is to use Long term we should move into animating all lines and areas at once if we want smoother enter/exit animations I think |
This PR now has some issues in development mode where the animation reverts when a new bundle is sent to the page 🙃 (save a file in dev mode) I'll try rendering all lines together using useTransition instead T_T |
I spent a lot of time trying to figure out how to use |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm surprised that this useMemo
change something.
Maybe adding a comments explaining what does it solves such that we do not remove it in a year without noticing the regression it would create
Me too 🙃 I added it when copying the behaviour of At one point I thought it had fixed #13450 but I just wasn't moving my mouse fast enough 😭 |
Fixes #14291
To the best of my knowledge, this doesn't seem to be an issue on
react-spring
itself, but an odd interaction with our lib.Other solutions I've tried
useReducedMotion
(no effect)useSpring
with a dependency array (no effect)animatedWidth
asstyle={{width:animatedWidth}}
(no effect)