Merging Hub into Editor #176
Conversation
| plotSchema: this.plotSchema, | ||
| plotly: this.props.plotly, | ||
| }; | ||
| } | ||
|
|
||
| updateProp(event) { | ||
| handleUpdate({type, payload}) { |
There was a problem hiding this comment.
We could go one step further here and split this handler into handleUpdateTraces, handleDeleteAnnotation etc... there's essentially no shared code here and no real need to have a standard {type, payload} pattern IMO.
There was a problem hiding this comment.
Is there a case where users will want to have their own update function? What if they want the updates to happen inside some reducer file, or apply updates and also do other things... If there are multiple functions it may be more difficult to replace?
| } | ||
| } | ||
|
|
||
| render() { | ||
| return ( | ||
| <div className={bem()}> | ||
| {this.props.graphDiv && | ||
| this.props.graphDiv._fullLayout && |
There was a problem hiding this comment.
this is a big change, conceptually, because it allows us to pass a partially-constructed figure into the editor and the plot, then the plot finishes constructing it.
| @@ -58,37 +54,22 @@ class App extends Component { | |||
| super(); | |||
|
|
|||
| // A basic starting plotly.js figure object. Instead of assigning | |||
| const figure = { | |||
| const graphDiv = { | |||
| data: [{type: 'scatter'}], | |||
| layout: {title: 'Room readings'}, | |||
| }; | |||
|
|
|||
There was a problem hiding this comment.
I'm not sure about this, when the plot component updates, it sets graphDiv to the actual dom element:
https://github.com/plotly/react-plotly.js/blob/master/src/factory.js#L167
it's that actual dom element that we're passing around and deriving all our values from, so making it seem like it is not the case is confusing to me
@bpostlethwaite is this not the case, am I missing something here?
There was a problem hiding this comment.
Also, I don't think I understand how that state.figure used to work and update, what was making it update?
There was a problem hiding this comment.
Plotly.js would basically copy the figure.data and figure.layout references into graphDiv and pass it to the Editor via the Hub... Now it's just more explicit that the App has a handle on graphDiv: it partially-constructs it (i.e. gives it data and layout) and then Plotly.js adds all the other stuff it needs to (basically by overwriting it every time it updates)
There was a problem hiding this comment.
but to me it seems like the graphDiv that plotly.js passes is the actual dom div element.. when looking at the code
There was a problem hiding this comment.
There was a problem hiding this comment.
Re the code here, we could also just have an initialFigure = {data, layout} type thing, with the initial value of graphDiv as null... I had it this way earlier and it worked as well, if we want to make it really explicit what's happening.
There was a problem hiding this comment.
what note would you add?
There was a problem hiding this comment.
that graphDiv is later on replaced by a full dom node with all the other properties of plotly.js
There was a problem hiding this comment.
Re the code here, we could also just have an initialFigure = {data, layout} type thing, with the initial value of graphDiv as null
I think I prefer that
Then no note is really needed :)
There was a problem hiding this comment.
I added a comment per your suggestion. The reason I removed the initialFigure thing is that in render() it looked really weird to have <Plot data={this.initialFigure.data} layout={this.initialFigure.layout} ... /> because it seemed like it would never get new data.
|
yup, alright the comment works for me, so on my end, we're good on this one |
|
OK, awesome :) Any thoughts on breaking up |
|
I don't mind |
|
maybe add react-plotly.js-editor in the package.json of the async example? ;) |
|
added to package.json |
|
@bpostlethwaite any additional comments or does this work for you, in terms of addressing your worst complaints about #173 ? |
|
Yep a single update function works for me. I am not sure how many users will need to override this behaviour yet or if that is a total non-issue. 💃 !!! |
Here's #175 retargeted onto #173 ... Meant to fix #172 :)