Empty folds #699

dmt0 · 2018-08-16T22:52:52Z

addresses #656
closes #695

also bumps plotly.js to 1.40.0

nicolaskruchten · 2018-08-17T01:48:07Z

src/components/containers/TraceAccordion.js

-    return this.filteredTraces.map((d, i) => {
-      return (
+    return (
+      this.filteredTraces.length > 0 &&


I think this changes the meaning... now when length === 0 we return false instead of null.

Yeah, but AFAIK behaviour stays the same

could we return null here if it doesn't meet this condition.

what difference would that make?

If null and false are both ignored by React then I'm fine with this. If React also ignores [] then we can just return the result of .map() without the guard.

nicolaskruchten · 2018-08-17T01:48:45Z

src/components/containers/TraceAccordion.js

+        return i;
+      } else if (
+        props.excludeNoColorbar &&
+        ((context.fullData[i].marker &&


this code is repeated in the PanelMenu piece that hides the ColorBar panel... can you think of a way to centralize it?

nicolaskruchten · 2018-08-17T01:50:11Z

src/components/containers/TraceAccordion.js

-        return !(t.transforms && t.transforms.every(tr => tr.type === 'fit'));
+    this.filteredTracesFullDataPositions = base.map((t, i) => {
+      if (
+        props.excludeFits &&


OK we have three filters here... can we just pass in one filter function and have the filter code defined at the call point rather than have this big if?

nicolaskruchten · 2018-08-17T01:50:49Z

src/lib/constants.js

  'box',
  'violin',
+  'bar',
+  'ohlc',


so here we've just added ohlc and candlestick, right?

yes, that's all that can be done safely, box and violin are already there

nicolaskruchten · 2018-08-17T02:08:30Z

With a pie as the first trace and a scatter as the second I get this:

With this error in the console:

Warning: Failed prop type: Invalid prop `traceIndexes[0]` of type `boolean` supplied to `TraceConnectedPlotlyFold`, expected `number`.
    in TraceConnectedPlotlyFold (created by TraceAccordion)
    in TraceAccordion (created by GraphTransformsPanel)
    in GraphTransformsPanel (created by DefaultEditor)

nicolaskruchten · 2018-08-17T02:09:30Z

Also when I have pie then scatter with a colorscale in the scatter, the Color Bar panel is present but empty.

VeraZab · 2018-08-17T13:41:53Z

src/components/containers/TraceAccordion.js

-      if (props.excludeFits) {
-        return !(t.transforms && t.transforms.every(tr => tr.type === 'fit'));
+    this.filteredTracesFullDataPositions = base.map((t, i) => {
+      if (


I don't agree with this logic.
When base is data, like in the Create panel, you cannot assume that the index of an item in the data array is the same as its index in the fullData array.

The case I can think of is in the Create Panel, which has base data,
if you add 1 split trace, and 1 non split case.

Data will be like: [{type: 'scatter'}, {type: 'pie'}]
FullData will be like: [{type: 'scatter', transforms: []}, {type: 'scatter', transforms: []}, {type: 'pie'} ]

In the Create panel, you'd be mapping on data, so you'd have a this.filteredTracesFulDataPositions of [0, 1], where you should have [0, 2].

nicolaskruchten · 2018-08-17T13:51:38Z

Well, no every exclusion has to apply to every case of grouped vs un grouped... this is why I would move the filter out to the call point

…

On Fri, Aug 17, 2018 at 09:41 VeraZab ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/components/containers/TraceAccordion.js <#699 (comment)> : > @@ -24,14 +24,36 @@ class TraceAccordion extends Component { // we don't want to include analysis transforms when we're in the create panel const base = props.canGroup ? context.fullData : context.data; - this.filteredTracesFullDataPositions = []; - this.filteredTraces = base.filter((t, i) => { - if (props.excludeFits) { - return !(t.transforms && t.transforms.every(tr => tr.type === 'fit')); + this.filteredTracesFullDataPositions = base.map((t, i) => { + if ( I don't agree with this logic. When base is data, like in the Create panel, you cannot assume that the index of an item in the data array is the same as its index in the fullData array. The case I can think of is in the Create Panel, which has base data, if you add 1 split trace, and 1 non split case. Data will be like: [{type: 'scatter'}, {type: 'pie'}] FullData will be like: [{type: 'scatter', transforms: []}, {type: 'scatter', transforms: []}, {type: 'pie'} ] In the Create panel, you'd be mapping on data, so you'd have a this.filteredTracesFulDataPositions of [0, 1], where you should have [0, 2]. — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#699 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAMbA4ucZj1wf8T_QgcNXY1f0hIyxb7tks5uRsghgaJpZM4WAs42> .

VeraZab · 2018-08-17T14:21:14Z

Sorry, I'm not sure I understand the comment @nicolaskruchten.

This code is in setLocals that gets called anytime new props are going to be received..
Then on render (the call point), we reuse what we've calculated from the latest props, so I think that we're passing every exclusion to renderTraceFolds, renderUngroupedFolds, renderGrouped folds.

But what I was saying above was that the filteredTracesFullDataPositions array, should be an array that represents the index of each of the this.filteredTraces in fullData. And with @dmt0 current changes, this is not what happens. :)

VeraZab · 2018-08-17T18:37:48Z

src/components/containers/TraceAccordion.js

    this.filteredTraces = base.filter((t, i) => {
-      if (props.excludeFits) {
-        return !(t.transforms && t.transforms.every(tr => tr.type === 'fit'));
+      if (traceFilterCondition(t, context.fullData[i])) {


context.fullData[i]
...well, base could be data or fullData, given this, do we still want to send context.fullData[i]?
or maybe just base[i] is ok?

we ARE sending base[i]. The second argument is a special case for Colorbar stuff. Doesn't get looked at by any other filter condition funcs.

VeraZab · 2018-08-17T18:41:19Z

src/components/containers/TraceAccordion.js

-      );
-    });
+      ))
+    );


same return null when doesn't meet condition

VeraZab · 2018-08-17T18:43:57Z

src/components/containers/TraceAccordion.js

        <TraceFold
          key={i}
-          traceIndexes={[i]}
+          traceIndexes={[this.filteredTracesFullDataPositions[i]]}


yeah... im not sure about this, here i think we'd want data indexes not fullData, why do we need that change?

traceIndexes should stay data related, that's what its meaning was throughout the code.

i is not index of data, it's index of a filteredTraces array. Perhaps I should make a filteredTracesDataPositions?

actually, filteredTracesFullDataPositions are not relative to fullData, they're relative to base which could be anything - and on that particular line base would be data

VeraZab · 2018-08-17T18:44:49Z

src/default_panels/StyleColorbarsPanel.js

-    <TraceAccordion
-      messageIfEmptyFold={_('Need a color scale for a colorbar!')}
-    >
+    <TraceAccordion traceFilterCondition={traceHasColorbar}>


yeah, this is much better on the api

VeraZab · 2018-08-17T18:46:58Z

Could you please look at the 2 comments with 😕 , those are my 2 potentially blocking comments.

VeraZab · 2018-08-17T19:36:38Z

💃

nicolaskruchten · 2018-08-17T19:56:39Z

💃 on behaviour.

dmt0 added 2 commits August 16, 2018 16:15

add transforms for more traces

6d639cf

bump plotly to 1.40

a10f405

dmt0 requested review from VeraZab and nicolaskruchten August 16, 2018 22:53

nicolaskruchten suggested changes Aug 17, 2018

View reviewed changes

VeraZab reviewed Aug 17, 2018

View reviewed changes

dmt0 force-pushed the empty-folds branch 3 times, most recently from 4231046 to 0a6b593 Compare August 17, 2018 17:45

VeraZab reviewed Aug 17, 2018

View reviewed changes

src/components/containers/TraceAccordion.js Outdated

);

});

))

);

Copy link

Contributor

VeraZab Aug 17, 2018

Choose a reason for hiding this comment

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

same return null when doesn't meet condition

VeraZab reviewed Aug 17, 2018

View reviewed changes

dmt0 force-pushed the empty-folds branch from 0a6b593 to 9256823 Compare August 17, 2018 19:12

no more empty folds

e6959e5

dmt0 force-pushed the empty-folds branch from 9256823 to e6959e5 Compare August 17, 2018 19:59

nicolaskruchten approved these changes Aug 17, 2018

View reviewed changes

dmt0 merged commit 9dd3d90 into master Aug 17, 2018

dmt0 deleted the empty-folds branch August 17, 2018 20:06

Uh oh!

Empty folds #699

Empty folds #699

Uh oh!

Conversation

dmt0 commented Aug 16, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicolaskruchten commented Aug 17, 2018

Uh oh!

nicolaskruchten commented Aug 17, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicolaskruchten commented Aug 17, 2018 via email

Uh oh!

VeraZab commented Aug 17, 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!

dmt0 Aug 17, 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 Aug 17, 2018

Uh oh!

VeraZab commented Aug 17, 2018

Uh oh!

nicolaskruchten commented Aug 17, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

dmt0 Aug 17, 2018 •

edited

Loading