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

Fix #260, allow '{}' as Graph.figure #285

Merged
merged 3 commits into from
Aug 30, 2018
Merged

Conversation

rmarren1
Copy link
Contributor

@rmarren1 rmarren1 commented Aug 28, 2018

released at dash-core-components==0.28.1

}

return PlotMethod(id, figure.data, figure.layout, config).then(
return Plotly.react(id, figure.data, figure.layout, config).then(
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you clarify this for me? Isn't Plotly.newPlot needed?

Copy link
Contributor 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, I just reverted this commit: ea4e66c#diff-b2faf9645a4fcca0badb59ef2fba58a3

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For context, that change was made because there was a bug in Plotly. That bug was fixed in plotly/plotly.js#2561 but we never removed the fix from Dash.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#170 I found this, this is why we call Plotly.react

Copy link
Member

Choose a reason for hiding this comment

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

For some more context:

  • newPlot is needed for the first plot
  • Subsequent plots can be done with Plotly.react. It was intended to the be faster way to handle updates but in reality we don't get many of the improvements because we are always providing new copies of data. Context here: Plotly.react plotly.js#2341. If users added a layout.datarevision, then they would get some performance improvements but I haven't tested this and it's not really documented.
  • So, while we don't get the main intended features of Plotly.react, we do get one very critical feature: the ability to repeatedly replot webgl plots: Repeatedly calling newPlot with scatter3d causes graphs to disappear plotly.js#2423
  • Now, there were a lot of bugs with Plotly.react when it first came out. One of the bugs was that candlestick and ohlc charts didn't work with it. So, I called newPlot just for those chart types
  • However, that bug has been fixed. So, we should standardize on all chart types using Plotly.react. It also might fix an issue that a community user was seeing in relayoutData fails with Candlestick plots  dash#355 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, great! Thanks for clarifying!

CHANGELOG.md Outdated
@@ -2,6 +2,10 @@
All notable changes to this project will be documented in this file.
This project adheres to [Semantic Versioning](http://semver.org/).

## [0.28.1]
### Fixed
- Fix bug where front-end error was thrown when setting `Graph.figure = {}` (fixes [#260]).
Copy link
Member

Choose a reason for hiding this comment

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

Could you add a ### Changed that mentions that "candlestick and OHLC charts are now plotted using the Plotly.react method instead of the Plotly.newPlot method."

CHANGELOG.md Outdated
@@ -2,6 +2,10 @@
All notable changes to this project will be documented in this file.
This project adheres to [Semantic Versioning](http://semver.org/).

## [0.28.1]
Copy link
Member

Choose a reason for hiding this comment

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

Could we also start adding dates here?

@chriddyp
Copy link
Member

Could we add a quick integration test that displays this behaviour? i.e. a simple callback that updates a figure by returning {}

@rmarren1
Copy link
Contributor Author

Made all those changes

@valentijnnieman
Copy link
Contributor

Cool! 💃

@chriddyp
Copy link
Member

💃

@rmarren1 rmarren1 merged commit 5cccfd3 into plotly:master Aug 30, 2018
@rmarren1
Copy link
Contributor Author

released at dash-core-components==0.28.1

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.

3 participants