New context API #1462
New context API #1462
Conversation
| static contextTypes = { | ||
| getTimer: PropTypes.func | ||
| }; | ||
| static contextType = TimerContext; | ||
|
|
||
| constructor(props) { |
There was a problem hiding this comment.
I found that React requires you to pass the context constructor argument to the super call in order for this.context to be set. Right now, this.context is always undefined.
| if (this.context.getTimer) { | ||
| return this.context.getTimer(); | ||
| if (this.context && this.context.globalTimer) { | ||
| return this.context.globalTimer; | ||
| } | ||
| if (!this.timer) { |
There was a problem hiding this comment.
With the new React context, the context should always default to the globalTimer defined in timer-context.js in case it is not set. Thus, you can assume that this value is set and as such you don't need logic for constructing a timer here.
|
It seems like |
|
Apparently, |
|
Interesting... Let me do some work to reconcile the timers. Thank you for helping out! |
|
@fabianishere Thank you so much for helping diagnose. I added explicitly separate timers for animations and transitions and everything now works as expected. I will end up merging this PR rather than the one you opened, but you are owed most of the credit for getting this improvement working. I will make sure the changelog calls out your contribution. |
@fabianishere
This is my attempt at using the new context API with the current animation model. It's mostly working except for stopping animation in VictoryZoomContainer. Also, the global timer does not seem to be reliably provided, necessitating keeping the find / create code in
getTimer().Heavily influenced by #1461
closes #1442