Skip to content

Modify Plotly.plot to accept frames #1014 #1054

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

Merged
merged 3 commits into from
Oct 19, 2016
Merged

Conversation

rreusser
Copy link
Contributor

@rreusser rreusser commented Oct 18, 2016

This PR overloads Plotly.plot so that loading frames isn't a second, promise-chained step.

Original (still works)

Plotly.plot(gd, data, layout, config)

Variation

Plotly.plot(gd, {
  data: [...]
  layout: {...}
  config: {...}
  frames: [...]
});

Now unnecessary

Plotly.plot(gd, data, layout, config).then(function () {
  return Plotly.addFrames(gd, frames);
});

@@ -330,6 +346,7 @@ Plotly.plot = function(gd, data, layout, config) {
Lib.syncOrAsync([
Plots.previousPromises,
drawFramework,
addFrames,
Copy link
Contributor

Choose a reason for hiding this comment

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

why after drawFramework?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, got it. Plots.supplyDefaults(gd); is called synchronously in the above function body so yeah, switching those two should be fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Switched.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(Of course you could now ask the opposite question, but I think loading data first is nicer.)

@etpinard etpinard added this to the v1.19.0 milestone Oct 18, 2016
layout: {width: 500, height: 500},
config: {editable: true},
frames: [{y: [2, 1, 0], name: 'frame1'}]
}).then(function() {
Copy link
Contributor

Choose a reason for hiding this comment

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

@rreusser does

Plotly.plot(gd, {
  data: [],
  frames: [ /* some initial list of frames */ ]
})
.then((gd) => { 

  // add more frames!!
  Plotly.addFrames(gd), [ /* more frames !! */ ]);
})

work as expected?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup!

Unexpected token )

But srsly, added this test and just pushed.

@etpinard
Copy link
Contributor

nicely done 💃

@rreusser rreusser merged commit bf714d0 into master Oct 19, 2016
@rreusser rreusser deleted the plot-call-signature branch October 19, 2016 17:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature something new
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants