Test nested connections and add nested connection #91

bpostlethwaite · 2017-11-20T02:47:14Z

@alexcjohnson This PR removes the modifyPlotProps system in favour of supplyPlotProps. It also introduces BoxGap - a component that talks to Layout though nested within a trace context.

supplyPlotProps is more general in that a component has the responsibility of returning plotProps rather than just modifying them. This adds a line of boilerplate to all components requiring additional behaviour const plotProps = unpackPlotProps(props, context); but the advantage is that there is more control.

For example <BoxGap> uses this functionality to forcibly assign fullLayout to fullContainer which gets around the problem of having a layoutConnected component inside a <Section> inside a traceConnected component.

The problem is that <Section> can only see the trace context so it hands unpackPlotProps a fullContainer that is fullData not fullLayout. The Layout connected component nested within does not recompute plotProps as we do not want to compute plotProps twice for every component in a <Section>. The Layout connected component is therefore broken. Now <BoxGap> supplies a layout fullContainer and we have a workaround.

PR review comment suggested that until override via props is necessary we implement only the necessary context

Instead of modifyPlotProps we have a more general supplyPlotProps. If a supplyPlotProps is present it is run. Most implementations of supplyPlotProps will run `unpackPlotProps` first and then alter plotProps as necessary. However, this enables us to now alter `props` and `context` _before_ running unpackPlotProps. This will allows us to "fix" the problem of nested layout fields within a trace context. See #58 (comment) for background.

alexcjohnson · 2017-11-20T17:17:56Z

src/lib/connectToContainer.js

-        WrappedComponent.modifyPlotProps(props, context, plotProps);
+    static supplyPlotProps(props, context) {
+      if (supplyPlotProps) {
+        return supplyPlotProps(props, context);


not blocking by any means, this will work fine, but for symmetry and to avoid multiple things with the bare name supplyPlotProps I might have written this as

if (config.supplyPlotProps) { return config.supplyPlotProps(props, context); }

True, that might be an easier read. I'll make the change

alexcjohnson · 2017-11-20T17:21:39Z

src/DefaultEditor.js

+              <BoxGap
+                label={_('Box Padding')}
+                attr="boxgroupgap"
+                showArrows={false}


Do you need this? It's already a NumericNoArrows component...

Nope! Forgot to take it out

alexcjohnson · 2017-11-20T17:29:58Z

src/DefaultEditor.js

@@ -145,6 +147,21 @@ class DefaultEditor extends Component {
              <ColorPicker label={_('Border Color')} attr="marker.line.color" />
            </TraceMarkerSection>

+            <Section name={_('Size and Spacing')}>
+              <BoxGap label={_('Bar Width')} attr="bargap" />


Hmm, what do we call this component? I feel like its name should indicate that it's numeric and that the attr you specify is in layout, not in a trace. But it's not specific to boxes or gaps.

Also, the label 'Bar Width' isn't quite right, is it? ie an increased bargap decreases the width, right?

TraceLayoutNumericSansArrows.
I copied the workspace descriptions but looks like it is doing a transformation on the input. Maybe (1-x)*100. I'll implement this transform (and the reverse onUpdate) in this PR and tag you on the commit.

With the transformation it will again be a hyper specific component so perhaps BoxGap is more applicable.

Though BoxWidth is probably clearer

bpostlethwaite · 2017-11-20T19:26:57Z

@alexcjohnson okay implemented your suggestions. Also cb1a95a reintroduces modifyPlotProps. More info in the commit message.

We now have modifyPlotProps and supplyPlotProps. We have need of both and trying to overload any one of them for both behaviours results in complicated code.

alexcjohnson · 2017-11-20T19:43:25Z

src/components/containers/Section.js

@@ -52,6 +52,9 @@ export default class Section extends Component {
      } else if (isAttr) {
        if (child.type.supplyPlotProps) {
          plotProps = child.type.supplyPlotProps(child.props, context);
+          if (child.type.modifyPlotProps) {
+            child.type.modifyPlotProps(child.props, context, plotProps);
+          }
        } else {
          plotProps = unpackPlotProps(child.props, context);


If you see a use for this unpack, then wouldn't the if (child.type.modifyPlotProps) block possibly be relevant in that case too? ie should it be moved outside that if/else block, so the existences of supply and modify are independent?

alexcjohnson · 2017-11-20T19:46:51Z

src/shame.js

+  });
+};
+
+export const BoxWidth = connectLayoutToPlot(


Perhaps FractionalWidth? Or LayoutWidthFraction? Violin is going to use this too.

Yep works for me 👍

something like 1bebf70 ?

alexcjohnson · 2017-11-20T19:51:03Z

src/shame.js

+        }
+      };
+
+      plotProps.max = 100;


right now all of these have min: 0, max: 1, so setting just max = 100 works. But I'd feel more comfortable about future-proofing if we at least explicitly supply min = 0, and maybe even better do the same (1-v) * 100 transform (which would actually switch min / max)

good point will do.

alexcjohnson · 2017-11-20T20:13:51Z

nice, lets go! 💃

bpostlethwaite added 4 commits November 18, 2017 21:28

AxesSelector only handles context

adfecc9

PR review comment suggested that until override via props is necessary we implement only the necessary context

test nesting layout within trace connection works as expected

74674e2

change default behaviour of test-utils deref and simplify tests

7ac3006

bpostlethwaite mentioned this pull request Nov 20, 2017

Section visibility nuances #58

Closed

utilize trace > layout connected workaround for <BoxGap>

04c83ef

bpostlethwaite force-pushed the testNestedConnectionsAndAddNestedConnection branch from a5f8d5e to 04c83ef Compare November 20, 2017 02:54

alexcjohnson reviewed Nov 20, 2017

View reviewed changes

bpostlethwaite added 2 commits November 20, 2017 13:15

remove unncessary showArrows. This is now taken care of by component

c3445bd

use config namespace to make it clear origin of supplyPlotProps

f1ce3ed

bpostlethwaite added 2 commits November 20, 2017 14:41

Reintroduce modifyPlotProps

92661db

We now have modifyPlotProps and supplyPlotProps. We have need of both and trying to overload any one of them for both behaviours results in complicated code.

use modify+supply funcs for BoxWidth and BoxPad

1c137f3

bpostlethwaite force-pushed the testNestedConnectionsAndAddNestedConnection branch from fad2cb5 to 1c137f3 Compare November 20, 2017 19:41

alexcjohnson reviewed Nov 20, 2017

View reviewed changes

add test to cover modifyPlotProps and nested Layout within Section

cd37d4c

alexcjohnson reviewed Nov 20, 2017

View reviewed changes

implementation specific naming for Box* + min max transformation

94a27c1

bpostlethwaite force-pushed the testNestedConnectionsAndAddNestedConnection branch from 1bebf70 to 94a27c1 Compare November 20, 2017 20:04

adds SCSS to build pipeline and remove unused deps

96d2ba0

bpostlethwaite merged commit 76efee6 into develop Nov 21, 2017

bpostlethwaite deleted the testNestedConnectionsAndAddNestedConnection branch November 21, 2017 14:54

This was referenced Nov 21, 2017

Test possibility of layout and data connected components #85

Closed

test Section wrapping a component using ModifiedPlotProps #83

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Test nested connections and add nested connection #91

Test nested connections and add nested connection #91

bpostlethwaite commented Nov 20, 2017

alexcjohnson Nov 20, 2017

bpostlethwaite Nov 20, 2017

alexcjohnson Nov 20, 2017

bpostlethwaite Nov 20, 2017

alexcjohnson Nov 20, 2017

bpostlethwaite Nov 20, 2017

bpostlethwaite Nov 20, 2017

bpostlethwaite Nov 20, 2017

bpostlethwaite commented Nov 20, 2017

alexcjohnson Nov 20, 2017

alexcjohnson Nov 20, 2017

bpostlethwaite Nov 20, 2017

bpostlethwaite Nov 20, 2017

alexcjohnson Nov 20, 2017

bpostlethwaite Nov 20, 2017

bpostlethwaite Nov 20, 2017

alexcjohnson commented Nov 20, 2017

Test nested connections and add nested connection #91

Test nested connections and add nested connection #91

Conversation

bpostlethwaite commented Nov 20, 2017

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

bpostlethwaite commented Nov 20, 2017

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

alexcjohnson commented Nov 20, 2017