Subplots2 #357

VeraZab · 2018-02-23T04:29:37Z

No description provided.

VeraZab · 2018-02-26T01:37:49Z

nicolaskruchten · 2018-02-26T12:55:55Z

src/components/fields/Field.js

@@ -58,6 +61,14 @@ class Field extends Component {
            </MenuPanel>
          ) : null}
        </div>
+        {action ? (


does this really belong in Field? Is it a generic enough thing that we'll want it in multiple fields?

well, it may :) if you don't mind too much I'd like to keep it in here for the moment

or looking at it again, yeah, I may change this, it's a bit ugly

well, it's likely not worth going through the effort of moving it now, but in future I'd be more comfortable adding in this kind of thing higher up in the stack, not in the base-level components like Field :)

nicolaskruchten · 2018-02-26T12:56:40Z

src/components/fields/derived.js

  supplyLayoutPlotProps,
  striptags,
 } from 'lib';

+export const AxisAnchorDropdown = connectToContainer(UnconnectedDropdown, {


general comment: we should start breaking up this file... it's long and getting longer as we add more and more derived types. Doesn't have to be done in this PR but soon. Maybe create an issue?

nicolaskruchten · 2018-02-26T12:58:21Z

src/components/fields/derived.js

-          plotly.Axes.list(graphDiv, props.attr.charAt(1))
-        ),
-      ];
+      if (props.attr === 'axref') {


for this and the cases below, we should DRY it up :) Something like a map over {x: "axref", y: "ayref"}

nicolaskruchten · 2018-02-26T12:59:31Z

src/lib/__tests__/multiValued-test.js

@@ -7,7 +7,7 @@ import {connectLayoutToPlot, connectAxesToLayout} from '..';
 import {mount} from 'enzyme';

 describe('multiValued Numeric', () => {
-  it.only('uses placeholder and empty string value', () => {


should this be looked-for and flagged in the syntax.js test?

this is just a test I uncommented, because my previous merge was messing up this behaviour and I knew I'd fix it by introducing subplots..

yeah, I meant that this kind of it.only should never be allowed right? like we don't allow fit and xit etc?

yeah it should not be allowed..

#362 will prevent you from trying this trick again ;)

nicolaskruchten · 2018-02-26T13:03:29Z

src/components/fields/AxisCreator.js

+import {connectToContainer, localize, traceTypeToAxisType} from 'lib';
+import {EDITOR_ACTIONS} from 'lib/constants';
+import {PlusIcon} from 'plotly-icons';
+


this component should show the axis title, not just the x, x2, y2 axis IDs

that's done, as per below gif

It's done in the AxesSelector but not the AxisCreator right?

nicolaskruchten · 2018-02-26T13:08:18Z

This looks pretty good! A couple of my comments need addressing... I've also noticed some odd behaviour in the UI itself but nothing specific I can point to as a bug just yet. Mostly it seems hard or impossible to build a basic dual-Y-axis chart with this UI unfortunately. Maybe I'm missing something I'll try to do some more systematic review after lunch. Sorry to be out of sync today.

VeraZab · 2018-02-27T02:46:54Z

hmm, i did set the y2 axis to be easy to make..

and i made axes name reflect the names they're given by the user:

nicolaskruchten · 2018-02-27T03:04:26Z

So if I create two traces and then set one of them to the second Y-axis, I only ever see 1 trace displayed, I'm not sure where the other one goes!

VeraZab · 2018-02-27T03:08:25Z

I only ever see 1 trace displayed, I'm not sure where the other one goes!

are you talking about the example in my gif, or some that you've tried?

nicolaskruchten · 2018-02-27T03:10:55Z

I'm trying it locally. If I set up trace 0 with x=col1, y=col1, then trace1 with x=col3, y=col3, then I set trace1's y-axis to a new axis, I see the following... where are the blue points for trace0?

VeraZab · 2018-02-27T03:21:18Z

hmm, you're right, ok, will investigate more of this tomorrow.. right now.. I'm not sure

nicolaskruchten · 2018-02-27T03:22:19Z

Yeah, it's weird, and not necessarily related to this PR, but worth investigating!

nicolaskruchten · 2018-02-27T03:31:16Z

Ah ok, it looks like yaxis2.overlaying = 'y' is needed... Not sure if forcing that on creation makes sense or not. We're already moving it to the right side right?

VeraZab · 2018-02-27T14:32:47Z

@alexcjohnson would you take a look at this pr for me? see if the logic makes sense.

Also, about creating a second y axis, I haven't noticed that part of my data disappears when I assign yaxis: 'y2' to a trace..
https://codepen.io/plotly/pen/648434b7b06688fc6b00067592d616b0
As Nicolas points out above, it seems like an overlaying: 'y' is needed for both traces to show up. Is this something that maybe plotly.js should be handling by default?

If it's not something to be handled by default by plotly.js, what's the best update that I need to send to plotly.js for all my traces to continue to show up after I've created new axes?

alexcjohnson · 2018-02-27T15:36:26Z

it seems like an overlaying: 'y' is needed for both traces to show up. Is this something that maybe plotly.js should be handling by default?

That would be tricky for plotly.js to figure out - we'd have to infer that you intend the two y axes to have not only the same domain but to actually be used within the same subplot (as opposed to just side-by-side plots). Theoretically possible, but I feel like it would be hard to make robust. Whereas here when you're making the second axis, you know whether the intent is overlaid, side-by-side, stacked, or whatever, and you can send yaxis2.overlaying='y' if that's the intent.

nicolaskruchten · 2018-02-27T15:39:30Z

@alexcjohnson I'm having trouble understanding the overlay: free behaviour... if y and y2 are both anchored to x, and both are overlay: free (the default) then why don't both traces show up on the plot?

alexcjohnson · 2018-02-27T15:43:56Z

src/components/containers/ImageAccordion.js

-      images.map((ann, i) => (
-        <ImageFold key={i} imageIndex={i} name={ann.text} canDelete={canAdd}>
+      images.map((img, i) => (
+        <ImageFold key={i} imageIndex={i} name={img.text} canDelete={canAdd}>


images don't have .text do they? Perhaps make a name from .source? Not sure what to do about shapes... index and type perhaps?

hehe so this change is @VeraZab cleaning up my copy-paste work from a previous PR... When I created the Images panel I'd based it off of the Notes panel but not renamed ann to img (and ditto Shapes). So any issues with this code should probably go in a separate issue that I can handle ;)

alexcjohnson · 2018-02-27T15:59:14Z

I'm having trouble understanding the overlay: free behaviour...

Heh that's because there isn't any free behavior. That's a mistake in the attribute definition, I'll fix it. If you look at _fullLayout the only options you should actually see are an axis id, undefined (there are no possible axes to overlay) or false (you declined to overlay another axis).

overlaying does two related but things when you enable it: 1) it forces the axis to copy the domain of the axis it's overlaying, and 2) it draws its data (when sharing an x axis too) on top of the data for the axis it's overlaying, without first covering the lower subplot in an opaque background.

It's helpful to remember the case you DO want one subplot to block another: inset plots. You do not want data from the main plot to show up behind the inset. I'm sure there could be smarter ways to handle this kind of case, since it's admittedly rare... but hopefully that at least makes it clear how we arrived at the situation we have now.

nicolaskruchten · 2018-02-27T16:02:28Z

Ahh, thanks for the explainer, it makes sense now :)

alexcjohnson · 2018-02-27T16:20:19Z

src/lib/getAllAxes.js

+                ? subplot.slice(0, 1) + 'axis' + subplot.slice(1)
+                : subplot + 'axis';
+
+            fullLayout[subplot].subplot = subplot;


If you're adding something to fullLayout that's not a real attribute, can you make it private, ie _subplot?

alexcjohnson · 2018-02-27T16:26:06Z

src/lib/connectAxesToLayout.js

@@ -2,14 +2,19 @@ import React, {Component} from 'react';
 import PropTypes from 'prop-types';
 import nestedProperty from 'plotly.js/src/lib/nested_property';
 import {deepCopyPublic, setMultiValuedContainer} from './multiValues';
-import {getDisplayName, localize, capitalize} from '../lib';
+import {capitalize, getAllAxes, getDisplayName, localize} from '../lib';


Also not new in this PR, but Isn't this from '.'? Actually, wouldn't it be better to import from the individual files anyway rather than lib/index, since this way is a circular import? It obviously doesn't break, but makes me uneasy...

alexcjohnson · 2018-02-27T17:05:46Z

src/lib/constants.js

+  gl3d: ['scatter3d', 'surface', 'mesh3d'],
+  geo: ['scattergeo', 'choropleth'],
+  mapbox: ['scattermapbox'],
+};


This feels fragile to me - an alternative that wouldn't require any hardcoding here would be to look at fullTrace._module.basePlotModule.name. Traces that don't have a subplot, like pie, will define their own basePlotModule, so then you can filter on names in the list of known subplot types.

I'll add this to our list of refactorings in fit n' finish.. thanks..

nicolaskruchten · 2018-02-27T20:12:42Z

src/components/fields/derived.js

+export const AxisOverlayDropdown = connectToContainer(UnconnectedDropdown, {
+  modifyPlotProps: (props, context, plotProps) => {
+    let options = [];
+    if (plotProps.fullContainer.subplot.includes('xaxis')) {


could this be DRYed up with a map on xaxis, yaxis ?

VeraZab · 2018-02-27T21:53:52Z

@nicolaskruchten would you checkout this branch once more and see if it looks ok to you..?

nicolaskruchten · 2018-03-01T21:13:08Z

src/lib/getAllAxes.js

+        fullLayout._subplots[type].forEach(subplot => {
+          if (['xaxis', 'yaxis'].includes(type)) {
+            // subplot will look like x2, x45, convert it to xaxis2, xaxis45
+            subplot = // eslint-disable-line no-param-reassign


why disable eslint here as opposed to just creating a new variable?

nicolaskruchten · 2018-03-01T21:16:09Z

💃 💃 ! thanks for putting up with my million comments on this one :)

VeraZab changed the title ~~Subplots2~~ [WIP] Subplots2 Feb 23, 2018

VeraZab force-pushed the subplots2 branch 3 times, most recently from 0e8d9be to 21ad5b0 Compare February 25, 2018 23:56

VeraZab changed the title ~~[WIP] Subplots2~~ Subplots2 Feb 26, 2018

nicolaskruchten suggested changes Feb 26, 2018

View reviewed changes

alexcjohnson reviewed Feb 27, 2018

View reviewed changes

nicolaskruchten reviewed Feb 27, 2018

View reviewed changes

nicolaskruchten mentioned this pull request Feb 27, 2018

block it.only and describe.only #362

Merged

VeraZab force-pushed the subplots2 branch from b49016d to a12f2ef Compare February 27, 2018 20:53

nicolaskruchten mentioned this pull request Feb 27, 2018

Fit'n'finish checklist #290

Closed

45 tasks

VeraZab force-pushed the subplots2 branch from a737666 to 43c69b7 Compare February 27, 2018 21:51

VeraZab force-pushed the subplots2 branch from 8d4bf80 to 7210c02 Compare March 1, 2018 20:40

VeraZab added 21 commits March 1, 2018 15:41

Min adjustment

707e90d

Export getAllAxes and adjust for subplots

816d282

Stop using child.type

5dd25e8

Create AxisCreator + Positionning controls

aee11ac

Correct axis selector axis names and display

fde5575

Only allow creation of new axes for cartesian types for now

3eddce8

Make y2 creation prettier

d7f425a

Correct annotation axesref

6ca5176

Remove noShame function

dd3e2ef

Standardize axis titles

0b543d7

Add Overlay Dropdown

c264f2a

Rebasing and test fix

9ac1b03

Adjust axis Side

cbfdb0d

Make .subplot && .axisGroup private keys

e12b272

Tests fix

c5bd06e

🙈

716f490

Rebase error

662c9fd

Refactor && add restrictions

fa6d53e

Style adjustments

ca05f19

Add axis overlaying adjustments like for yaxis

c02573b

Make relationship between overlaying and domain clearer

d81e812

VeraZab force-pushed the subplots2 branch from 7210c02 to d81e812 Compare March 1, 2018 20:41

VeraZab added 2 commits March 1, 2018 15:53

Overlaying adjustment

6943cce

Linting

f8c86ed

nicolaskruchten reviewed Mar 1, 2018

View reviewed changes

nicolaskruchten approved these changes Mar 1, 2018

View reviewed changes

VeraZab merged commit be01eb4 into master Mar 1, 2018

VeraZab deleted the subplots2 branch March 1, 2018 21:16

Subplots2 #357

Subplots2 #357

Conversation

VeraZab commented Feb 23, 2018 • edited Loading

VeraZab commented Feb 26, 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

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

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

Choose a reason for hiding this comment

nicolaskruchten commented Feb 26, 2018

VeraZab commented Feb 27, 2018

nicolaskruchten commented Feb 27, 2018

VeraZab commented Feb 27, 2018 • edited Loading

nicolaskruchten commented Feb 27, 2018

VeraZab commented Feb 27, 2018

nicolaskruchten commented Feb 27, 2018

nicolaskruchten commented Feb 27, 2018

VeraZab commented Feb 27, 2018

alexcjohnson commented Feb 27, 2018

nicolaskruchten commented Feb 27, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexcjohnson commented Feb 27, 2018

nicolaskruchten commented Feb 27, 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

VeraZab commented Feb 27, 2018

Choose a reason for hiding this comment

nicolaskruchten commented Mar 1, 2018

VeraZab commented Feb 23, 2018 •

edited

Loading

VeraZab commented Feb 27, 2018 •

edited

Loading