Change default export to combined EditorControls + Plot component #384

nicolaskruchten · 2018-03-07T03:03:18Z

I'm happy to rename the new component but the idea here is to provide something that, though it keeps a gross DOM node in state, looks and behaves like a dumb component which bundles an editor and a plot area.

nicolaskruchten · 2018-03-07T03:04:22Z

dev/App.js

    this.state = {
-      graphDiv: {},
-      plotRevision: 0,
+      data: [],


note the new state in App: just data and layout!

nicolaskruchten · 2018-03-07T03:04:37Z

dev/App.js

+          dataSources={dataSources}
+          dataSourceOptions={dataSourceOptions}
+          plotly={plotly}
+          onUpdate={(data, layout) => this.setState({data, layout})}


note the new update call: just data and layout!

nicolaskruchten · 2018-03-07T03:05:16Z

src/PlotlyEditor.js

@@ -69,7 +69,7 @@ class PlotlyEditor extends Component {
          this.props.afterUpdateTraces(payload);
        }
        if (this.props.onUpdate) {
-          this.props.onUpdate();
+          this.props.onUpdate(graphDiv.data, graphDiv.layout);


in this iteration, we mutate and pass along. In the next iteration, we'll finally use immutability-helper to NOT mutate and pass along copies as per #212 :)

nicolaskruchten · 2018-03-07T03:09:29Z

src/PlotlyEditorWithPlot.js

+class PlotlyEditorWithPlot extends Component {
+  constructor(props) {
+    super();
+    this.state = {graphDiv: {}};


The new component only stores the graphDiv in state, which it gets from Plot.

So the lifecycle of a user edit is now:

data/layout from PlotlyEditor up to PlotlyEditorWithPlot up to App

setState causes render for App

data/layout from App down to PlotlyEditorWithPlot down to Plot

graphDiv from Plot up to PlotlyEditorWithPlot

setState causes render for PlotlyEditorWithPlot

graphDiv from PlotlyEditorWithPlot down to PlotlyEditor

bpostlethwaite

Cool lets try it out! Since you just built a great end-to-end testing machine lets get a couple tests in with this PR so we have some patterns to build off of.

Then we can start adding more tests with less friction!

bpostlethwaite · 2018-03-07T12:51:12Z

here is the batch reporter end-to-end test https://github.com/plotly/plotly.js-batch-reporter/blob/master/example/src/index.test.js

There are a few accompanying helper functions. One of which https://github.com/plotly/plotly.js-batch-reporter/blob/master/example/src/index.test.js#L11 may be instructive.

bpostlethwaite · 2018-03-07T12:53:55Z

also since things are async I use await https://github.com/plotly/plotly.js-batch-reporter/blob/master/example/src/index.test.js#L77 plus an async type of "waits for" routine called stateChange https://github.com/plotly/plotly.js-batch-reporter/blob/master/example/src/lib/testUtils.js#L78

VeraZab · 2018-03-07T13:18:09Z

This is what we suspected could make the editor really slow right? And it isn't really slow?
For names: Editor, Plot, PlotWithEditor?
But this looks good to me!

nicolaskruchten · 2018-03-07T16:09:56Z

@VeraZab Well, I didn't think it would make things "really slow" I said there might be a performance hit from extra renders. It doesn't feel bad to me at all though :)

nicolaskruchten · 2018-03-07T16:10:44Z

Actually the bigger performance hit probably came from #381

nicolaskruchten · 2018-03-07T19:31:56Z

@VeraZab @bpostlethwaite here's my more radical proposal: let's change the top-level export to be this new combined component that looks more React-idiomatic. I've renamed the top-level component to PlotlyEditor and the old one to EditorControls in this PR.

nicolaskruchten · 2018-03-07T19:32:34Z

If we like it, I can fix all the other examples and write some tests against it. I'm pretty convinced that this is a better way forward but we can discuss in person when @VeraZab is back :)

VeraZab · 2018-03-07T19:34:42Z

I like it! I feel like API users should not have to think about those things (most of them). And for the ones who do want to handle this in a more individual way, that's still available to them.

nicolaskruchten · 2018-03-07T20:18:58Z

OK so remaining to do on this PR:

add an end to end testing framework stub
fix the README
fix the existing examples

nicolaskruchten · 2018-03-08T18:55:43Z

@VeraZab check out how nice the examples are now! simple and redux :)

VeraZab · 2018-03-08T19:04:40Z

examples/demo/src/App.js

+          useResizeHandler
+          debug
+          advancedTraceTypeSelector
+        />


yeahhh!! I like it!! 🎉

VeraZab · 2018-03-08T19:06:39Z

package.json

@@ -20,7 +20,7 @@
    "raf": "^3.4.0",
    "react-color": "^2.13.8",
    "react-colorscales": "^0.4.2",
-    "react-plotly.js": "^1.6.0",
+    "react-plotly.js": "^1.7.0",
    "react-rangeslider": "^2.2.0",
    "react-select": "^1.0.0-rc.10",
    "react-tabs": "^2.2.1",


on ajoute le nouveau plotly.js aussi?

vdh · 2018-03-09T01:08:25Z

src/PlotlyEditor.js

-            this.props.graphDiv._fullLayout &&
-            (this.props.children ? this.props.children : <DefaultEditor />)}
-        </ModalProvider>
+      <div className="app">


Should app here be replaced with a BEM class instead? (Perhaps by adjusting/moving the one now in EditorControls?)

Good point, I just copied stuff over but app is a terrible class to bake in ;)

nicolaskruchten · 2018-03-12T20:08:54Z

@VeraZab @bpostlethwaite ready for review. The end-to-end test is a stub but it works and can be built upon in future PRs :)

partly addresses #321

VeraZab · 2018-03-12T20:17:09Z

src/PlotlyEditor.js

+        <div
+          className="plotly_editor_plot"
+          style={{width: '100%', height: '100%'}}
+        >


could these encoded styles go in the plotly_editor_plot className?

VeraZab · 2018-03-12T20:18:08Z

src/PlotlyEditor.js

+            onInitialized={graphDiv => this.setState({graphDiv})}
+            onUpdate={graphDiv => this.setState({graphDiv})}
+            style={{width: '100%', height: '100%'}}
+          />


and this too, can we not hard code here?

VeraZab · 2018-03-12T20:33:16Z

💃

bpostlethwaite · 2018-03-12T20:41:37Z

💃

nicolaskruchten requested review from bpostlethwaite and VeraZab March 7, 2018 03:03

nicolaskruchten force-pushed the combocomponent branch from f8472af to 91c9cef Compare March 7, 2018 03:03

nicolaskruchten commented Mar 7, 2018

View reviewed changes

This was referenced Mar 7, 2018

graphDiv masks hidden dependencies and complicates state management #355

Closed

PlotlyEditor handleUpdate mutates inputs instead of following unidirectional data flow #356

Open

bpostlethwaite requested changes Mar 7, 2018

View reviewed changes

nicolaskruchten force-pushed the combocomponent branch from 41da20d to abb0746 Compare March 7, 2018 19:20

nicolaskruchten force-pushed the combocomponent branch from 8d735bb to 8a5bac7 Compare March 8, 2018 17:25

VeraZab reviewed Mar 8, 2018

View reviewed changes

examples/demo/src/App.js

useResizeHandler

debug

advancedTraceTypeSelector

/>

Copy link

Contributor

VeraZab Mar 8, 2018

Choose a reason for hiding this comment

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

yeahhh!! I like it!! 🎉

VeraZab reviewed Mar 8, 2018

View reviewed changes

nicolaskruchten force-pushed the combocomponent branch 2 times, most recently from b21456f to f29c2f6 Compare March 8, 2018 20:01

vdh reviewed Mar 9, 2018

View reviewed changes

nicolaskruchten force-pushed the combocomponent branch from f29c2f6 to 34681a7 Compare March 9, 2018 20:55

nicolaskruchten added 2 commits March 12, 2018 11:32

first stab at combined component

6f98f23

track new version of react-plotly.js

80b359d

nicolaskruchten added 5 commits March 12, 2018 11:32

renaming reshuffle

16a3d4c

fix test

61f8f17

fixing up examples

c663ac5

readme and props passthrough

858201b

CSS for combo component

9168d7c

nicolaskruchten force-pushed the combocomponent branch from 3b92cfc to 9168d7c Compare March 12, 2018 15:33

nicolaskruchten added 2 commits March 12, 2018 11:59

tweaking examples

1cd9f67

end to end test stub

2e5c4e4

nicolaskruchten changed the title ~~[WIP] first stab at combined component~~ Change default export to combined EditorControls + Plot component Mar 12, 2018

VeraZab reviewed Mar 12, 2018

View reviewed changes

nicolaskruchten merged commit f207cb2 into master Mar 12, 2018

nicolaskruchten deleted the combocomponent branch March 12, 2018 20:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change default export to combined EditorControls + Plot component #384

Change default export to combined EditorControls + Plot component #384

nicolaskruchten commented Mar 7, 2018 •

edited

Loading

nicolaskruchten Mar 7, 2018

nicolaskruchten Mar 7, 2018

nicolaskruchten Mar 7, 2018 •

edited

Loading

nicolaskruchten Mar 7, 2018

bpostlethwaite left a comment

bpostlethwaite commented Mar 7, 2018

bpostlethwaite commented Mar 7, 2018 •

edited

Loading

VeraZab commented Mar 7, 2018

nicolaskruchten commented Mar 7, 2018

nicolaskruchten commented Mar 7, 2018

nicolaskruchten commented Mar 7, 2018

nicolaskruchten commented Mar 7, 2018

VeraZab commented Mar 7, 2018 •

edited

Loading

nicolaskruchten commented Mar 7, 2018 •

edited

Loading

nicolaskruchten commented Mar 8, 2018

VeraZab Mar 8, 2018

VeraZab Mar 8, 2018

nicolaskruchten Mar 8, 2018

vdh Mar 9, 2018

nicolaskruchten Mar 9, 2018

nicolaskruchten commented Mar 12, 2018

VeraZab Mar 12, 2018

VeraZab Mar 12, 2018

VeraZab commented Mar 12, 2018

bpostlethwaite commented Mar 12, 2018

Change default export to combined EditorControls + Plot component #384

Change default export to combined EditorControls + Plot component #384

Conversation

nicolaskruchten commented Mar 7, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolaskruchten Mar 7, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bpostlethwaite left a comment

Choose a reason for hiding this comment

bpostlethwaite commented Mar 7, 2018

bpostlethwaite commented Mar 7, 2018 • edited Loading

VeraZab commented Mar 7, 2018

nicolaskruchten commented Mar 7, 2018

nicolaskruchten commented Mar 7, 2018

nicolaskruchten commented Mar 7, 2018

nicolaskruchten commented Mar 7, 2018

VeraZab commented Mar 7, 2018 • edited Loading

nicolaskruchten commented Mar 7, 2018 • edited Loading

nicolaskruchten commented Mar 8, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolaskruchten commented Mar 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VeraZab commented Mar 12, 2018

bpostlethwaite commented Mar 12, 2018

nicolaskruchten commented Mar 7, 2018 •

edited

Loading

nicolaskruchten Mar 7, 2018 •

edited

Loading

bpostlethwaite commented Mar 7, 2018 •

edited

Loading

VeraZab commented Mar 7, 2018 •

edited

Loading

nicolaskruchten commented Mar 7, 2018 •

edited

Loading