mui-x icon indicating copy to clipboard operation
mui-x copied to clipboard

[charts] Fix `LineChart` not properly animating when hydrating

Open JCQuintas opened this issue 1 year ago • 6 comments

Fixes https://github.com/mui/mui-x/issues/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

  • Rewriting our useReducedMotion (no effect)
  • use useSpring with a dependency array (no effect)
  • pass animatedWidth as style={{width:animatedWidth}} (no effect)
  • have a prop that updates the component on client (is rendered fully on server, but re-plays animation on hydration)
    const [skipAnimation, setSkipAnimation] = React.useState(true)
    
    React.useEffect(() => setSkipAnimation(false), [])
    
    const { animatedWidth } = useSpring({
      from: { animatedWidth: left },
      to: { animatedWidth: width + left + right },
      reset: false,
      immediate: skipAnimation,
    });
    

JCQuintas avatar Aug 26 '24 18:08 JCQuintas

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

Generated by :no_entry_sign: dangerJS against dc352049d460fa79e710829ee99a656018e4b8bd

mui-bot avatar Aug 26 '24 18:08 mui-bot

CodSpeed Performance Report

Merging #14355 will not alter performance

Comparing JCQuintas:fix-linechart-client-side-render-issue (dc35204) with master (cf441ec)

Summary

✅ 3 untouched benchmarks

codspeed-hq[bot] avatar Aug 26 '24 18:08 codspeed-hq[bot]

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 build/serve a ssr app on playwright, then we could try to detect if the animation played 🤔, though it might be too much work 😅

JCQuintas avatar Aug 27 '24 13:08 JCQuintas

We could have a ci step that build/serve a ssr app on playwright, then we could try to detect if the animation played 🤔, though it might be too much work 😅

For sure it's to much work since we nearly never touch those animation

alexfauquette avatar Aug 27 '24 14:08 alexfauquette

https://github.com/mui/mui-x/pull/14366 introduced an issue on hydration that further made the current behaviour worse for hydrated line charts 😓

The current solution is to use useTransition instead of useSpring, reason in unknown...

Long term we should move into animating all lines and areas at once if we want smoother enter/exit animations I think

JCQuintas avatar Aug 28 '24 12:08 JCQuintas

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

JCQuintas avatar Aug 28 '24 13:08 JCQuintas

I spent a lot of time trying to figure out how to use useTransition for all lines, when simply memoing the plotted data seemed to be enough 😢

JCQuintas avatar Aug 29 '24 12:08 JCQuintas

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 BarPlot in order to add useTransition for all the lines, and I noticed the chart stopped re-rendering on mouse-over. So there might be an issue with the context that handles highligh or mouse movement.

At one point I thought it had fixed https://github.com/mui/mui-x/issues/13450 but I just wasn't moving my mouse fast enough 😭

JCQuintas avatar Aug 29 '24 13:08 JCQuintas