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

First pass at updating API for allowing animations #339

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

nicrummel
Copy link

Update display function to allow for frames for animation, and add animation example

@nicrummel
Copy link
Author

I see that the Labeler is failing, is there a way I can address this?

Copy link
Member

@sglyon sglyon left a comment

Choose a reason for hiding this comment

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

Thank you for making an attempt at this! It will be a welcome and helpful contribution.

I have one question about the change to calling the javascript Plotly.newPlot function, but other than that this looks great.

src/display.jl Outdated
@@ -97,7 +97,7 @@ function SyncPlot(

# Draw plot in container
Plotly.newPlot(
gd, $(lowered[:data]), $(lowered[:layout]), $(options)
gd, $(lowered), $(options)
Copy link
Member

Choose a reason for hiding this comment

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

Does this work as expected?

From the plotly.js docs (ref) it looks like we can use one of two forms of the newPlot function:

Plotly.newPlot(graphDiv, data, layout, config)

Plotly.newPlot(graphDiv, obj)

Here I don't believe we are using either

@nicrummel
Copy link
Author

I looked at the documentation, and I believe you caught a mistake. I have fixed the call in the most recent commit.

@sglyon
Copy link
Member

sglyon commented Aug 18, 2020

Now that the PlotlyBase side has been merged, hopefully I'll be able to review this one soon.

Thanks for the contributions and for the patience!

@nicrummel
Copy link
Author

nicrummel commented Sep 10, 2020

Dr. Lyon,
I see you have been quite busy with the development of other aspects of PlotlyJS.jl. Is ther anything I can do to help get this pull request along?

  • Possibly remove the example and docs that I made? Should I move those to another location?

  • I could also rebaise my branch on the current master. Would that help?

Regards,
Nic

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.

2 participants