Skip to content

Change default export to combined EditorControls + Plot component #384

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.

Sign up for GitHub

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

nicolaskruchten merged 9 commits into master from combocomponent

Mar 12, 2018

Merged

Change default export to combined EditorControls + Plot component #384

nicolaskruchten merged 9 commits into master from combocomponent

Mar 12, 2018

Conversation

nicolaskruchten

Copy link

Contributor

@nicolaskruchten nicolaskruchten commented Mar 7, 2018 •

edited

Loading

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 nicolaskruchten requested review from bpostlethwaite and VeraZab

March 7, 2018 03:03

@nicolaskruchten nicolaskruchten force-pushed the combocomponent branch from f8472af to 91c9cef Compare

March 7, 2018 03:03

nicolaskruchten

nicolaskruchten commented

View reviewed changes

this.state = {

graphDiv: {},

plotRevision: 0,

data: [],

Copy link

Contributor Author

@nicolaskruchten nicolaskruchten Mar 7, 2018

Choose a reason for hiding this comment

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

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

nicolaskruchten

nicolaskruchten commented

View reviewed changes

dev/App.js Outdated

dataSources={dataSources}

dataSourceOptions={dataSourceOptions}

plotly={plotly}

onUpdate={(data, layout) => this.setState({data, layout})}

Copy link

Contributor Author

@nicolaskruchten nicolaskruchten Mar 7, 2018

Choose a reason for hiding this comment

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

note the new update call: just data and layout!

nicolaskruchten

nicolaskruchten commented

View reviewed changes

src/PlotlyEditor.js Outdated

@@ -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);

Copy link

Contributor Author

@nicolaskruchten nicolaskruchten Mar 7, 2018 •

edited

Loading

Choose a reason for hiding this comment

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

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

nicolaskruchten commented

View reviewed changes

src/PlotlyEditorWithPlot.js Outdated

class PlotlyEditorWithPlot extends Component {

constructor(props) {

super();

this.state = {graphDiv: {}};

Copy link

Contributor Author

@nicolaskruchten nicolaskruchten Mar 7, 2018

Choose a reason for hiding this comment

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

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

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

View reviewed changes

Copy link

Member

@bpostlethwaite bpostlethwaite left a comment

Choose a reason for hiding this comment

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

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

Copy link

Member

bpostlethwaite commented Mar 7, 2018

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

Copy link

Member

bpostlethwaite commented Mar 7, 2018 •

edited

Loading

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

Copy link

Contributor

VeraZab commented Mar 7, 2018

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

Copy link

Contributor Author

nicolaskruchten commented Mar 7, 2018

@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

Copy link

Contributor Author

nicolaskruchten commented Mar 7, 2018

Actually the bigger performance hit probably came from #381

@nicolaskruchten nicolaskruchten force-pushed the combocomponent branch from 41da20d to abb0746 Compare

March 7, 2018 19:20

@nicolaskruchten

Copy link

Contributor Author

nicolaskruchten commented Mar 7, 2018

@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

Copy link

Contributor Author

nicolaskruchten commented Mar 7, 2018

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 :)

Copy link

Contributor

VeraZab commented Mar 7, 2018 •

edited

Loading

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

Copy link

Contributor Author

nicolaskruchten commented Mar 7, 2018 •

edited

Loading

OK so remaining to do on this PR:

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

@nicolaskruchten nicolaskruchten force-pushed the combocomponent branch from 8d735bb to 8a5bac7 Compare

March 8, 2018 17:25

@nicolaskruchten

Copy link

Contributor Author

nicolaskruchten commented Mar 8, 2018

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

VeraZab reviewed

View reviewed changes

examples/demo/src/App.js

useResizeHandler

debug

advancedTraceTypeSelector

/>

Copy link

Contributor

@VeraZab 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

View reviewed changes

package.json Outdated

@@ -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",

Copy link

Contributor

@VeraZab VeraZab Mar 8, 2018

Choose a reason for hiding this comment

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

on ajoute le nouveau plotly.js aussi?

Copy link

Contributor Author

@nicolaskruchten nicolaskruchten Mar 8, 2018

Choose a reason for hiding this comment

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

sure

@nicolaskruchten nicolaskruchten force-pushed the combocomponent branch 2 times, most recently from b21456f to f29c2f6 Compare

March 8, 2018 20:01

vdh

vdh reviewed

View reviewed changes

src/PlotlyEditor.js Outdated

this.props.graphDiv._fullLayout &&

(this.props.children ? this.props.children : <DefaultEditor />)}

</ModalProvider>

<div className="app">

Copy link

@vdh vdh Mar 9, 2018

Choose a reason for hiding this comment

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

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

Copy link

Contributor Author

@nicolaskruchten nicolaskruchten Mar 9, 2018

Choose a reason for hiding this comment

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

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

@nicolaskruchten 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

@nicolaskruchten


 first stab at combined component

6f98f23

@nicolaskruchten


 track new version of react-plotly.js

80b359d

nicolaskruchten added 5 commits

March 12, 2018 11:32

@nicolaskruchten


 renaming reshuffle

16a3d4c

@nicolaskruchten


 fix test

61f8f17

@nicolaskruchten


 fixing up examples

c663ac5

@nicolaskruchten


 readme and props passthrough

858201b

@nicolaskruchten


 CSS for combo component

9168d7c

@nicolaskruchten 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

@nicolaskruchten


 tweaking examples

1cd9f67

@nicolaskruchten


 end to end test stub

2e5c4e4

@nicolaskruchten nicolaskruchten changed the title ~~(削除) [WIP] first stab at combined component (削除ここまで)~~ (追記) Change default export to combined EditorControls + Plot component (追記ここまで)

@nicolaskruchten

Copy link

Contributor Author

nicolaskruchten commented Mar 12, 2018

@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 reviewed

View reviewed changes

src/PlotlyEditor.js

<div

className="plotly_editor_plot"

style={{width: '100%', height: '100%'}}

>

Copy link

Contributor

@VeraZab VeraZab Mar 12, 2018

Choose a reason for hiding this comment

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

could these encoded styles go in the plotly_editor_plot className?

VeraZab reviewed

View reviewed changes

src/PlotlyEditor.js

onInitialized={graphDiv => this.setState({graphDiv})}

onUpdate={graphDiv => this.setState({graphDiv})}

style={{width: '100%', height: '100%'}}

/>

Copy link

Contributor

@VeraZab VeraZab Mar 12, 2018

Choose a reason for hiding this comment

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

and this too, can we not hard code here?

Copy link

Contributor

VeraZab commented Mar 12, 2018

💃

1 similar comment

@bpostlethwaite

Copy link

Member

bpostlethwaite commented Mar 12, 2018

💃

@nicolaskruchten nicolaskruchten merged commit f207cb2 into master

@nicolaskruchten nicolaskruchten deleted the combocomponent branch

March 12, 2018 20:42

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

4 participants

@nicolaskruchten @bpostlethwaite @VeraZab @vdh