refactor: Heatmap chart instance management #749
Conversation
There was a problem hiding this comment.
This is fantastic! I really appreciate your help with this. It looks good to me; however, I have one question, which I've added as a comment. Thanks again, @tihuan!
| /** | ||
| * (thuang): Use this ref to store the onEvents prop to prevent | ||
| * unnecessary re-renders when the onEvents prop changes. | ||
| * NOTE: This implies that `onEvents` prop changes alone from the parent |
There was a problem hiding this comment.
Could this potentially cause issues later on?
I mean the fact that changing the onEvents won't trigger re-renders as you've mentioned!
There was a problem hiding this comment.
Yeah I was debating about this too 🤔
Really about finding a balance between UX (asking users to useCallback() and useMemo() to memoize their onEvents object, vs. performance (re-render every time onEvents changes or not)
I think in this case it feels like the event handlers are less likely to change (besides users re-creating the object every time) than data being passed, so I feel it's probably an okay tradeoff here?
Also I think the current implementation in this PR also ignores all the prop changes except for [echartsRendererMode] as well, so changing onEvents also won't re-render the chart, so behavior wise it should be the same as the current PR!
Summary
Structural Element (Base, Gene, DNA, Chromosome or Cell)
Github issue: #XXXX
Copy issue description here
Checklist
defaultTheme.tsused wherever possible