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

Gauge re-initialized whenever parent re-renders (needle animates from 0) #81

Open
RobRendell opened this issue Aug 20, 2021 · 12 comments
Open
Labels
help wanted Extra attention is needed

Comments

@RobRendell
Copy link

RobRendell commented Aug 20, 2021

I don't know why this is a problem only I appear to be seeing, but when the parent of a GaugeChart component re-renders due to prop changes, the GaugeChart needle resets to 0 and then animates to its current value, causing it to oscillate wildly when there are frequent prop changes. This happens even if the props passed to GaugeChart don't change (the parent renders other components as well). I'm using useMemo to ensure the props to GaugeChart like arcsLength and colors don't change.

I've verified (using an onEffect hook) that the parent is only re-rendering, and not being unmounted and re-mounted.

I did a bit of debugging in the GaugeChart code, and I was able to see that this call to initChart() with no parameters inside the useLayoutEffect hook was being called every time the parent's props changed. The dependencies listed for the useLayoutEffect hook include the whole props object for the GaugeChart component - doesn't that mean that the callback passed to useLayoutEffect will be re-invoked every time the GaugeChart props change?

I was able to stop the behaviour in my application by rolling back to [email protected]. The introduction of the useLayoutEffect hook with props in its dependencies seems to have happened with merge request #74.

@Martin36 Martin36 added the help wanted Extra attention is needed label Aug 21, 2021
@Martin36
Copy link
Owner

@stevenhankin

@bjbrewster
Copy link

bjbrewster commented Oct 6, 2021

I noticed the same and other issues. I'll be pushing a merge request that cleans this up a lot. The simple fix for this is to memoise the gauge props used by the useLayoutEffect like

const memoProps = React.useMemo(() => props, [JSON.stringify(props)])

And use memoProps instead of props. I would recommend pulling out just the create gauge props and memoising that instead of all the props though.

Also, the useDeepCompareMemo fn seems broken too. I believe line 10 should be if (!isDeepEquals(.... Currently it's updating the memoised deps if equal?
https://github.com/Martin36/react-gauge-chart/blob/master/src/lib/GaugeChart/customHooks.js#L10

@rossdeane
Copy link

Did you ever get round to doing a PR for this @bjbrewster? 🤞

@bjbrewster
Copy link

bjbrewster commented Feb 17, 2022 via email

@faizankhan1995
Copy link

@bjbrewster Is this merged ?

@raymar
Copy link

raymar commented May 19, 2022

@bjbrewster any (good) news regarding the PR?

@bjbrewster
Copy link

I'm actively working on this and have been testing it lots on our website. I will push this in the next few days. Please bare with me. I've added some tests to make sure my refactoring doesn't break anything. I would appreciate a few people trying it out after I've pushed it. I will aim to push by the end of this weekend.

@raymar
Copy link

raymar commented May 25, 2022

I'm actively working on this and have been testing it lots on our website. I will push this in the next few days. Please bare with me. I've added some tests to make sure my refactoring doesn't break anything. I would appreciate a few people trying it out after I've pushed it. I will aim to push by the end of this weekend.

@bjbrewster let me know once you are done, I will do some tests.

@ShayAxelrod
Copy link

I'm guessing you're still doing tests and haven't made the PR? This single feature underwent more testing than Cyberpunk 2077 😂😂

@inactivist
Copy link

inactivist commented Dec 8, 2022

@bjbrewster Any chance you can create a pull request for your fixes? Is there anything we can do to assist?

@bjbrewster
Copy link

bjbrewster commented Dec 20, 2022 via email

@antoniolago
Copy link

antoniolago commented May 26, 2023

Hello everyone, I have grown a great interest in this exceptional project and have made several modifications on my own. One of them includes a complete overhaul of the rendering process, which potentially resolves the issues you mentioned (and also #126, #44 (I'm planning to make some new types), #31, #27, #100, #57 and #122). Instead of submitting a pull request, I decided to create a new repository due to the disruptive changes and crazy experiments I'm still doing, feel free to check it out and test the modifications, let me know what u find.

You can find the repository here:
https://github.com/antoniolago/react-gauge-component

Cheers to Martin36 and all collaborators for creating this great project, not easy to find a free and nice gauge component.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

9 participants