Proper trace type initialization in TraceSelector component #196

VeraZab · 2018-01-02T22:57:11Z

No description provided.

VeraZab · 2018-01-03T00:07:21Z

ok, so I removed mode: 'markers' from examples/simple/App.js in the initial figure, as well as in PlotlyEditor.js' EDITOR_ACTIONS.ADD_TRACE.

Instead, on componentWillMount, will initialize the right value in the TraceSelector dropdown because I am calling updatePlot with the right Plotly.js initialization attributes. I am not calling it on componentWillReceive props because this.setLocals calls it, internally.

I've changed the plotlyTraceToCustomTrace function to take gd.data instead of gd._fullData, in that way I'm only going to evaluate to a line trace if trace.mode exists in gd.

@bpostlethwaite, is this good?

bpostlethwaite · 2018-01-03T00:34:19Z

package.json

@@ -13,27 +13,19 @@
    "url": "https://github.com/plotly/react-plotly.js-editor/issues"
  },
  "scripts": {
-    "make:combined-translation-keys":


Does your prettifier act on json as well?

yes, it shouldn't? It was from the start..

Hmmm I haven't configured prettier to handle JSON in this lib. Not sure which rule sets it would be applying. I was imagining only targeting stuff in src. It wouldn't be a bad thing to prettify the json but we don't have CI tests to test this so our styling could end up fighting without an arbiter.

bpostlethwaite · 2018-01-03T00:35:14Z

src/components/fields/TraceSelector.js

+  componentWillMount() {
+    const {container} = this.props;
+    const value = plotlyTraceToCustomTrace(container);
+    this.updatePlot(value);


we only want to do this if the mode isn't set though right? Also you are setting a custom trace type in user data which will break soon as they take their plot out of the editor app

don't we want to aggressively set this in componentWillReceiveProps? What if the user at some random future time supplies {x: [1], y: [1]} in the gd.data?

I'm calling updatePlot every time TraceSelector mounts, to make sure, that in any situation, the proper defaults are sent to plotly.js.

So, in the case that a user starts out with x: [], y: [], type: 'scatter', before TraceSelector mounts, updatePlot will be called with {x: [], y: [], type: 'scatter', mode: 'markers'}, that way, on the next call to updatePlot, if the user provides new props of x: [1], y: [1], mode will already have been set to 'markers'

This is the reason I don't need to call this in componentWillReceiveProps, is this ok?

It is usually better practice to only send an update if you have to send an update but it probably won't be that costly so most likely won't make much of a difference.

If you only call the function on component mount how will we aggressively repair a trace {x: [1], y: [1]} that comes into being on gd.data when this component is already mounted? If it is already mounted it won't send another update and this new trace will not be modified.

It is usually better practice to only send an update if you have to send

I DO have an update to send, I want to send the mode update, so that regardless of whether I have an empty state or a full one, the mode is set in gd.data.

So when x: [], y: [] changes to x:[1], y: [1] nothing additional needs to happen in componentWillReceiveProps because componentDidMount already set the mode in gd.data, and its gd.data that I use for trace recognition.

I'll slack you. I don't think I am communicating this effectively

VeraZab · 2018-01-03T14:49:23Z

Actually, there's a situation unaccounted for here: when the user actually wants mode: 'lines+markers' and they didn't think to set mode in gd.data because it's the plotly.js default, they didn't think they needed to.

So now, if we're using the editor in streambed, how am I supposed to differentiate between having to change mode on figure initialization, and not changing anything because the loaded figure actually wanted a trace with mode: 'lines+markers'

In streambed we initialize our figure explicitly with the mode we want, we should do so here too. As for the empty state x: [], y: [], if we're initializing our figure with a mode, it will be in gd.data, and since we're now looking in gd.data not fullData to recognize trace types, we will recognize traces correctly.

So I propose to change back to explicit mode setting in figure initialization, and that along with checking gd.data when recognizing trace types, should prevent us from mixing up trace types.
@bpostlethwaite, @alexcjohnson I'll let you see if you think of any objections to this.

streambed initialization: https://github.com/plotly/streambed/blob/2d9bd322e177c2eb3f4206ea95b86984247be4cd/shelly/webapp/static/webapp/src/workspace/reducers/partialStateWorkspace.js#L399

alexcjohnson · 2018-01-03T15:55:26Z

So I propose to change back to explicit mode setting in figure initialization, and that along with checking gd.data when recognizing trace types, should prevent us from mixing up trace types.

Good catch @VeraZab - I think this is what you're saying, but just to be sure, it seems like there are two distinct cases based on how we arrive at the figure:

Loading a saved plot with scatter traces missing mode: we should aggressively set mode to whatever it would have been based on the current plotly.js logic. If we find a blank trace, we can set that default to anything but I'd probably vote for 'lines+markers'.
Adding a new trace or changing to a scatter type: we always provide a mode as part of initializing the trace, based on the kind of trace you say you're adding/altering.

VeraZab · 2018-01-03T19:31:41Z

yup @alexcjohnson correct, that is what I'm saying. I'll make the changes now.

VeraZab · 2018-01-03T23:09:44Z

so, just so that we're sure that everything's updating correctly, I've tried adding a call to updatePlot just like in componentWillMount, but when I try to do so, I'm caught in an infinite loop, the apps don't render.

I think that it may have to do with the lifecycle of react-select, but honestly, I'm a little mixed up at this point, this call to componentWillReceiveProps doesn't seem to be needed.

I see that the TraceSelector component rerenders even when DataSelector components are changed, so I can see that rerendering works correctly, even if I'm not exactly sure how this is happening.

so I've pushed up what I've got so far, which seems to be working, if @bpostlethwaite you see a situation that this fix doesn't cover, could you give me an example, so that I can fix something more concrete. I would have added a call to updatePlot in componentWillReceiveProps, but it's breaking the app.

bpostlethwaite · 2018-01-03T23:24:26Z

what were the conditions you were you calling updatePlot() in componentWillReceiveProps?

bpostlethwaite · 2018-01-03T23:25:44Z

if you don't put a conditional around it then it will fire, the app will rerender, and it will fire again and yeh infinite loop.

But if you have something like

if (!container.mode) {
    updatePlot({mode: 'markers'})
}

Then shouldn't it reach a stable state on second time around? Ie this conditional will fail and we won't send a new update.

bpostlethwaite · 2018-01-03T23:30:29Z

Then again there is this chilling warning on this pattern https://hackernoon.com/evil-things-you-do-with-redux-dispatch-in-updating-lifecycle-methods-ad116de882d4

bpostlethwaite · 2018-01-03T23:34:48Z

How else could we do this... We do this to some effect in webapp.

    componentWillReceiveProps(nextProps) {
        if (!nextProps.sourcesRequest) {
            nextProps.onFetchSources();
        }
    }

type patterns.

VeraZab · 2018-01-03T23:47:15Z

🙈 forgot about the conditional in componentWillReceiveProps..

bpostlethwaite · 2018-01-03T23:47:49Z

Does this PR make the default line or scatter? Seems to be line, but in the webapp we pick scatter no?

VeraZab · 2018-01-03T23:48:50Z

mode has to be explicitly set to 'markers', because now we're covering the situation of a plot loaded through api, where the user might have intended to have mode: 'lines+makers'

bpostlethwaite · 2018-01-03T23:53:18Z

What if plotly defaults to markers instead of lines+markers though? I don't see any place in this code where we run the same algorithm plotly.js uses to make that decision.

VeraZab · 2018-01-04T02:07:26Z

ok, I think I've got it now
In batch editor:

In simple example:

bpostlethwaite · 2018-01-04T14:02:55Z

src/components/fields/__tests__/DataSelector-test.js

@@ -38,7 +38,7 @@ describe('DataSelector', () => {
    const onUpdateTraces = jest.fn();
    const wrapper = render({onUpdateTraces}).find(DropdownWidget);
    wrapper.prop('onChange')('y1');
-    expect(onUpdateTraces.mock.calls[0][0]).toEqual({
+    expect(onUpdateTraces.mock.calls[2][0]).toEqual({


You could clear the mock after the render right before calling onChange. That way we are impervious to these changes in indexes.

VeraZab · 2018-01-04T15:36:32Z

So here where this stands now.
I realized I can't change mode aggressively, because then I will have trouble recognizing when we're trying to use plotly defaults vs editor defaults. I need the case where mode only exists in fullData, to recognize that I want to use plotly.js default types.

This is important when the user switches from one trace to another.

Also, Ben pointed out yesterday that I should be using the plotly.js logic to determine trace.mode, which isn't always 'lines+markers': https://github.com/plotly/plotly.js/blob/master/src/traces/scatter/defaults.js#L32
I thought it's easily accessible on fullData.mode, but things get a bit inconsistent when switching chart types..

I'm reworking this so that mode isn't aggressively set.

VeraZab · 2018-01-04T18:19:52Z

Sorry for all the noise in this pr.
Somehow along the way I started diverging a little.

So, this pr responds to these requirements:

When a user loads a plot that has no mode in gd.data, we proactively set the mode ourselves in gd.data according to what's in gd.fullData (when the component initializes and on componentWillReceiveProps), so that our plotlyTraceToCustomTrace function in the editor, properly recognizes which trace type is loaded
When the user changes trace types in the editor, we use our editor defaults:
line: mode -> lines
scatter: mode -> markers
When the user starts out and has no data, we can't use fullData to get a mode on our initial empty trace, there's no data points which are used to populate mode in gd.fullData, so we use our editor default mode -> markers

@bpostlethwaite @alexcjohnson would you review please.

alexcjohnson · 2018-01-04T18:30:20Z

src/components/fields/TraceSelector.js

+  setTraceDefaults(container, fullContainer, updateContainer) {
+    if (
+      (container.type === 'scatter' && !container.mode) ||
+      (!container.type && fullContainer.type === 'scatter')


doesn't look quite right to me... so if container.type is missing, we'll set mode even if there already is one?
And if container.type is missing, isn't fullContainer.type always going to be 'scatter'?

I would have thought just !container.mode && (container.type === 'scatter' || !container.type)

!container.mode && (container.type === 'scatter' || !container.type)

you're right, changing this

alexcjohnson · 2018-01-04T18:34:42Z

src/components/fields/TraceSelector.js

+    ) {
+      updateContainer({
+        type: 'scatter',
+        mode: fullContainer.mode || 'markers',


I don't have a real strong opinion here, but I probably would have gone for 'lines+markers' as that's the "<20 points" default and we get here only (I think?) when there are no points.

I put markers, because it's the editor default..
We're using plotly.js defaults wherever we can with fullContainer.mode, but when we're at our first trace that is empty and didn't have any data put in yet, we put in the editor default of 'markers'

bpostlethwaite

Yes looks good to me! 💃 We can tweak the defaults a little if we have to before we hit v1.0.

alexcjohnson · 2018-01-04T19:08:30Z

src/lib/customTraceType.js

    ['tozeroy', 'tozerox', 'tonexty', 'tonextx', 'toself', 'tonext'].includes(
      trace.fill
    )
  ) {
    return 'area';
  } else if (
    trace.type === 'scatter' &&
+    trace.mode &&
    (trace.mode === 'lines' || trace.mode === 'lines+markers')


are && trace.fill and && trace.mode actually necessary here? falsey values will fail the following clauses anyway.

yeah..you're right, removing

alexcjohnson

looks great! 💃 (though you'll probably need another after rebasing...)

alexcjohnson

💃

VeraZab · 2018-01-04T19:15:56Z

thanks!!

VeraZab force-pushed the properly-initialize-trace-type branch 2 times, most recently from 0e2cc1a to 5cc424c Compare January 2, 2018 23:55

bpostlethwaite reviewed Jan 3, 2018

View reviewed changes

VeraZab force-pushed the properly-initialize-trace-type branch from 7eb3e19 to f59614c Compare January 3, 2018 22:47

VeraZab force-pushed the properly-initialize-trace-type branch from 42a2c98 to 3a58812 Compare January 4, 2018 13:58

bpostlethwaite reviewed Jan 4, 2018

View reviewed changes

VeraZab force-pushed the properly-initialize-trace-type branch 2 times, most recently from 33f4495 to f793f7a Compare January 4, 2018 18:05

alexcjohnson reviewed Jan 4, 2018

View reviewed changes

bpostlethwaite previously approved these changes Jan 4, 2018

View reviewed changes

alexcjohnson reviewed Jan 4, 2018

View reviewed changes

VeraZab dismissed bpostlethwaite’s stale review via 6dce0ce January 4, 2018 19:10

alexcjohnson previously approved these changes Jan 4, 2018

View reviewed changes

VeraZab added 5 commits January 4, 2018 14:13

Fix trace Delete typo

e877e79

Force mode when !trace.mode, but _fullTrace.mode is present

9621b23

Add/adjust tests

89943e8

Review comments

b207430

Review 2

9398c33

VeraZab dismissed alexcjohnson’s stale review via 9398c33 January 4, 2018 19:13

VeraZab force-pushed the properly-initialize-trace-type branch from 6dce0ce to 9398c33 Compare January 4, 2018 19:13

alexcjohnson approved these changes Jan 4, 2018

View reviewed changes

VeraZab merged commit b236f50 into master Jan 4, 2018

VeraZab deleted the properly-initialize-trace-type branch January 4, 2018 19:16

Uh oh!

Proper trace type initialization in TraceSelector component #196

Proper trace type initialization in TraceSelector component #196

Uh oh!

Conversation

VeraZab commented Jan 2, 2018

Uh oh!

VeraZab commented Jan 3, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

VeraZab Jan 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

VeraZab commented Jan 3, 2018

Uh oh!

alexcjohnson commented Jan 3, 2018

Uh oh!

VeraZab commented Jan 3, 2018

Uh oh!

VeraZab commented Jan 3, 2018

Uh oh!

bpostlethwaite commented Jan 3, 2018

Uh oh!

bpostlethwaite commented Jan 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bpostlethwaite commented Jan 3, 2018

Uh oh!

bpostlethwaite commented Jan 3, 2018

Uh oh!

VeraZab commented Jan 3, 2018

Uh oh!

bpostlethwaite commented Jan 3, 2018

Uh oh!

VeraZab commented Jan 3, 2018

Uh oh!

bpostlethwaite commented Jan 3, 2018

Uh oh!

VeraZab commented Jan 4, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

VeraZab commented Jan 4, 2018

Uh oh!

VeraZab commented Jan 4, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bpostlethwaite left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexcjohnson left a comment

Choose a reason for hiding this comment

Uh oh!

VeraZab Jan 3, 2018 •

edited

Loading

bpostlethwaite commented Jan 3, 2018 •

edited

Loading