Skip to content
This repository was archived by the owner on Jun 3, 2024. It is now read-only.

Add frames to Graph #540

Merged
merged 1 commit into from
Apr 30, 2019
Merged

Add frames to Graph #540

merged 1 commit into from
Apr 30, 2019

Conversation

nicolaskruchten
Copy link
Contributor

frames are part of the Plotly.js figure spec, so dcc.Graph() should accept them.

@nicolaskruchten
Copy link
Contributor Author

I'm not sure what Percy is up to here... @alexcjohnson what's your recommendation? rerun? approve?

@alexcjohnson
Copy link
Collaborator

We've seen that percy diff before, have to track it down at some point but it's OK for now.

Does this PR work though? Doesn't look like frames are propagated to Plotly.react

return Plotly.react(id, figure.data, clone(figure.layout), config).then(

in fact we have to use the other signature Plotly.react(gd, {data, layout, frames, config}) in order to use frames, right?

@nicolaskruchten
Copy link
Contributor Author

Good point, I hadn't actually dug in that far :)

@nicolaskruchten
Copy link
Contributor Author

Do you think I should clone frames or not?

@alexcjohnson
Copy link
Collaborator

Shouldn’t need cloning, no.

@nicolaskruchten nicolaskruchten force-pushed the nicolaskruchten-patch-1 branch from bab0d52 to 601b386 Compare April 29, 2019 19:42
@nicolaskruchten
Copy link
Contributor Author

OK so this one actually works now @alexcjohnson :)

@nicolaskruchten
Copy link
Contributor Author

I've approved that same Percy diff, so this one should be good to review.

@nicolaskruchten
Copy link
Contributor Author

Do you want a changelog entry here also?

@nicolaskruchten nicolaskruchten force-pushed the nicolaskruchten-patch-1 branch from 601b386 to 1caa259 Compare April 30, 2019 17:28
@nicolaskruchten
Copy link
Contributor Author

OK I added one.

@nicolaskruchten nicolaskruchten force-pushed the nicolaskruchten-patch-1 branch from 1caa259 to 9c324ba Compare April 30, 2019 18:35
Copy link
Collaborator

@alexcjohnson alexcjohnson left a comment

Choose a reason for hiding this comment

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

Looks good! 💃
I'd prefer including frames in a test, but if you want to get this in quick I'm OK without it - just know I'm going to forward any frames bugs to you 😏

@nicolaskruchten
Copy link
Contributor Author

🤝 deal.

@nicolaskruchten nicolaskruchten merged commit 4153430 into master Apr 30, 2019
@nicolaskruchten nicolaskruchten deleted the nicolaskruchten-patch-1 branch April 30, 2019 18:43
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants