Replace addEvents HOC with custom hook
#2007
Conversation
| @@ -13,11 +13,13 @@ | |||
| "@babel/transform-spread", | |||
| "@babel/transform-template-literals", | |||
| "@babel/proposal-object-rest-spread", | |||
| "@babel/plugin-proposal-export-namespace-from" | |||
| "@babel/plugin-proposal-export-namespace-from", | |||
| "@babel/plugin-proposal-optional-chaining" | |||
There was a problem hiding this comment.
I'm still getting warnings due to this plugin - I will either need to fix webpack to resolve these warnings or take out this plugin.
|
|
||
| export default addEvents(VictoryBar); | ||
| return <VictoryBar {...props} />; | ||
| }; |
There was a problem hiding this comment.
I'm not sure whether this is the right solution, but we still needed some sort of wrapper component in order to access the VictoryTransition props in the main component.
There was a problem hiding this comment.
If it stays, it probably needs a better name.
| }, [sharedEvents]); | ||
|
|
||
| const baseProps = React.useMemo(() => { | ||
| const sharedParentState = getSharedEventState(KEYS.PARENT, KEYS.PARENT); |
There was a problem hiding this comment.
This is basically a copy of addEvents for now, but long term I would love to move away from getting this state from the sharedEvents prop. Passing in sharedEvents is causing additional re-rendering, as it is updating the props for all the data components regardless of whether each individual component is changing. It also makes it difficult to memoize props, like in #2006 where I needed to iterate through children to take out this prop. I may continue to work on this and create some sort top-level shared events context rather than using this prop.
This is the work I have done so far to refactor the
addEventsHOC to a custom hook in order to get a better handle on how many times the parent component is re-rendering. This is implemented in VictoryBar, and works with events and animations.