Section visibility nuances

[bpostlethwaite]

see #54 (comment)

Right now <Section> will hide itself when all of its attr children are not visible. Currently if you put non attr children in a <Section> it will show. However you could make the case that <Info> (for example) should not by default force <Section> to show since it is <Info> about non applicable (and hidden) fields.

We might be able to extend this argument to <MenuPanel> (CogMenu) as well. If CogMenu is always (or almost always) an advanced settings of the base fields in <Section> then it may not make sense to show it.

To resolve this we need to look at all the CogMenus in the workspace and figure out sane defaults.

[bpostlethwaite]

Note that right now if you click legend HIDE the positioning field still shows up as MenuPanel triggers section visibility . We should turn that off and test.

[bpostlethwaite]

<Section> is passing in its context to unpackPlotProps which may be different than the context received by the children. For example if you have a structure like this:

<TraceConnectedPanel />
    <Section />
        <LayoutConnectedField />

<Section> will only see TraceConnected context objects and unpackPlotProps will return a different value then what will be computed by its <LayoutConnectedField> child. This is really bad... since the visibility will be computed incorrectly.

This is blocking adding layout attributes inside a Tracepanel. For example boxgap and friends.

cc @alexcjohnson sorta stuck on this one. We may have to rethink the way visibility is handled...

[bpostlethwaite]

https://medium.com/@mweststrate/how-to-safely-use-react-context-b7e343eff076 Hmmm we may have to rethink our use of context period.

[bpostlethwaite]

One way would be to also define defaultProps for the container attr like fullLayout or fullData. Then the <Section> will see that prop and could use it. This would be set on the higher order component like connectLayoutToPlot.

I'll investigate that.

[bpostlethwaite]

So this is all <Section>s fault. We need to work around the fact that <Section> precomputes plotProps using the wrong context. This has 2 negative side effects.

First is that it just computes the wrong visibility for the field. Second, once it has computed the wrong plotProps the default behaviour of a connectToContainer component is to not recompute them but simply used the ones passes to it by <Section> (or whatever parent). This saves some computation for all the components nested in <Section> but bites us here.

One way to get around the visibility problem is with a modifyPlotProps function in the higher order component that wraps container nested fields. This has been used a bunch of other times to customize the behaviour of certain fields. To work around the second case we will need to signal to connectToContainer that it needs to recompute plotProps.

I am going to move some of the other simple exceptions to a shame.js file and add the layout-inside-trace <Gap> widgets in there as well. Once I have progressed further it will hopefully become more apparent what exceptions need to be refactored away.

[bpostlethwaite]

Also re: shouldComponentUpdate and context. This is pretty bad overall but if we are clear that any non-Editor intermediary component that is inserted into the graph and wants to implement shouldComponentUpdate needs to check the Editor.revision number we should be ok. If it increases the component must update. Then whenever we change context we make sure the revision number is always incremented. The rudiments for this are already in place. See #61

[bpostlethwaite]

workaround here #91

[bpostlethwaite]

closed by #99