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

Migrate to new context API #1461

Closed
wants to merge 2 commits into from
Closed

Migrate to new context API #1461

wants to merge 2 commits into from

Conversation

fabianishere
Copy link

@fabianishere fabianishere commented Dec 19, 2019

Hi!

When running an application in StrictMode, I stumbled upon the warning that Victory is still using the legacy context API (#1442). This pull requests removes the usage of the legacy API and adds two new context types TimerContext and PortalContext, which carry respectively the application's global timer and the portal context functions.

Some parts of the code are not well-tested (yet), so might not be implemented correctly. In particular, no test touches the code in zoom-helpers.js. In this case, I assume the context passed to the function is the class instance of the component (where this.timer is set).

Hope this is helpful!

Fixes #1442

@boygirl
Copy link
Contributor

boygirl commented Dec 19, 2019

@fabianishere I definitely appreciate the help here. I checked out your work locally, though, and it looks like this change breaks all animations. I will need to dig in more to understand why this is happening. I've had a worry for some time now that Victory's animation model might need a more significant overhaul to work correctly with the new context API :(

@fabianishere
Copy link
Author

fabianishere commented Dec 19, 2019

Interesting, that did not occur to me. Any way to reproduce this? If I recall correctly, there are no tests for animation in the storybook.

I may be able to dig into this if you would like.

This change updates several components to pass down the global Timer
object via the new React context API.
This change changes the internal Portal code to make use of a
PortalContext using the new React context API.
@boygirl
Copy link
Contributor

boygirl commented Dec 19, 2019

@fabianishere you can run the demo pages w/ yarn start and navigate to localhost:3000. You'll see a list of demos, including a page of animations here: http://localhost:3000/#/animation. Apologies for not including these in the storybook tests. They aren't testable with the screenshot testing service we use.

I'm actually taking a look at this right now, and iterating from your work. I'll push something in the next ~hour. I would love an extra set of eyes.

@fabianishere
Copy link
Author

fabianishere commented Dec 19, 2019

@boygirl I just pushed another version. I noticed the context was not being set correctly in victory-transition.js causing errors when using animation. I retried using the demos and I see some of the animations are working, but not all.

EDIT: Apparently the animations work when reloading the page.

if (!this.context.getTimer) {
this.getTimer().stop();
}
this.timer.stop();
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure whether this is the correct translation here. Should the timer only be stopped by this component if it has created the timer itself?

@boygirl boygirl mentioned this pull request Dec 19, 2019
@boygirl
Copy link
Contributor

boygirl commented Dec 20, 2019

Closing in favor of #1462. Thank you @fabianishere for all the help!

@boygirl boygirl closed this Dec 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

React.StrictMode support? (migrate to new context API)
2 participants