remove visibility computations from modifyPlotProps, do it higher up

VeraZab · VeraZab · commit d118796375ab · 2019-09-13T14:13:54.000-04:00
diff --git a/src/components/containers/PlotlySection.js b/src/components/containers/PlotlySection.js
@@ -1,6 +1,10 @@
 import React, {Component} from 'react';
 import PropTypes from 'prop-types';
-import {containerConnectedContextTypes, unpackPlotProps} from '../../lib';
+import {
+  containerConnectedContextTypes,
+  unpackPlotProps,
+  validateVisibilityWithCustomConfig,
+} from '../../lib';
 
 export class Section extends Component {
   constructor() {
@@ -45,7 +49,7 @@ export default class PlotlySection extends Section {
 
   determineVisibility(nextProps, nextContext) {
     const {isVisible} = unpackPlotProps(nextProps, nextContext);
-    this.sectionVisible = Boolean(isVisible);
+    this.sectionVisible = validateVisibilityWithCustomConfig(isVisible, nextProps, nextContext);
 
     React.Children.forEach(nextProps.children, child => {
       if (!child || this.sectionVisible) {
@@ -57,7 +61,13 @@ export default class PlotlySection extends Section {
         if (child.type.modifyPlotProps) {
           child.type.modifyPlotProps(child.props, nextContext, plotProps);
         }
-        this.sectionVisible = this.sectionVisible || plotProps.isVisible;
+
+        this.sectionVisible = validateVisibilityWithCustomConfig(
+          this.sectionVisible || plotProps.isVisible,
+          child.props,
+          nextContext,
+          child.type && child.type.displayName ? child.type.displayName : null
+        );
         return;
       }
 
diff --git a/src/components/fields/HoverLabelNameLength.js b/src/components/fields/HoverLabelNameLength.js
@@ -1,6 +1,6 @@
 import React, {Component} from 'react';
 import PropTypes from 'prop-types';
-import {connectToContainer, hasValidCustomConfigVisibilityRules} from 'lib';
+import {connectToContainer} from 'lib';
 import Field from './Field';
 import RadioBlocks from '../widgets/RadioBlocks';
 import NumericInput from '../widgets/NumericInput';
@@ -86,11 +86,8 @@ UnconnectedHoverLabelNameLength.displayName = 'UnconnectedHoverLabelNameLength';
 export default connectToContainer(UnconnectedHoverLabelNameLength, {
   modifyPlotProps: (props, context, plotProps) => {
     const {container} = plotProps;
-    const baseRule =
+    plotProps.isVisible =
       (container.hoverinfo && container.hoverinfo.includes('name')) ||
       (container.hovertemplate || container.hovertemplate === ' ');
-    plotProps.isVisible = hasValidCustomConfigVisibilityRules(context.customConfig)
-      ? plotProps.isVisible && baseRule
-      : baseRule;
   },
 });
diff --git a/src/components/fields/LocationSelector.js b/src/components/fields/LocationSelector.js
@@ -1,10 +1,6 @@
 import React, {Component} from 'react';
 import PropTypes from 'prop-types';
-import {
-  connectToContainer,
-  hasValidCustomConfigVisibilityRules,
-  computeCustomConfigVisibility,
-} from 'lib';
+import {connectToContainer} from 'lib';
 import Field from './Field';
 import Radio from './Radio';
 import {UnconnectedDropdown} from './Dropdown';
@@ -14,9 +10,7 @@ const LocationmodeVisible = connectToContainer(UnconnectedDropdown, {
   modifyPlotProps: (props, context, plotProps) => {
     if (!plotProps.fullValue) {
       plotProps.fullValue = plotProps.container.locationmode;
-      plotProps.isVisible = hasValidCustomConfigVisibilityRules(context.customConfig)
-        ? computeCustomConfigVisibility(props, plotProps.fullValue, context.customConfig)
-        : true;
+      plotProps.isVisible = true;
 
       return;
     }
diff --git a/src/components/fields/derived.js b/src/components/fields/derived.js
@@ -11,14 +11,7 @@ import Info from './Info';
 import {UnconnectedColorPicker} from './ColorPicker';
 import {UnconnectedTextEditor} from './TextEditor';
 import {UnconnectedVisibilitySelect} from './VisibilitySelect';
-import {
-  computeCustomConfigVisibility,
-  connectToContainer,
-  getAllAxes,
-  getAxisTitle,
-  axisIdToAxisName,
-  hasValidCustomConfigVisibilityRules,
-} from 'lib';
+import {connectToContainer, getAllAxes, getAxisTitle, axisIdToAxisName} from 'lib';
 
 export const AxisAnchorDropdown = connectToContainer(UnconnectedDropdown, {
   modifyPlotProps: (props, context, plotProps) => {
@@ -87,9 +80,7 @@ export const RangesliderVisible = connectToContainer(UnconnectedRadio, {
     if (!plotProps.fullValue) {
       plotProps.fullValue = false;
       plotProps.visible = false;
-      plotProps.isVisible = hasValidCustomConfigVisibilityRules(context.customConfig)
-        ? computeCustomConfigVisibility(props, plotProps.fullValue, context.customConfig)
-        : true;
+      plotProps.isVisible = true;
       return;
     }
   },
@@ -171,10 +162,7 @@ export const TickFormat = connectToContainer(UnconnectedDropdownCustom, {
 export const ShowInLegend = connectToContainer(UnconnectedVisibilitySelect, {
   modifyPlotProps: (props, context, plotProps) => {
     if (context.container.type && context.container.type !== 'sunburst') {
-      const baseRule = context.fullLayout.showlegend;
-      plotProps.isVisible = hasValidCustomConfigVisibilityRules(context.customConfig)
-        ? plotProps.isVisible && baseRule
-        : baseRule;
+      plotProps.isVisible = context.fullLayout.showlegend;
     }
 
     return plotProps;
@@ -183,32 +171,23 @@ export const ShowInLegend = connectToContainer(UnconnectedVisibilitySelect, {
 
 export const HistogramInfoVertical = connectToContainer(Info, {
   modifyPlotProps: (props, context, plotProps) => {
-    const baseRule =
+    plotProps.isVisible =
       context.fullContainer.type === 'histogram' && context.fullContainer.orientation === 'v';
-    plotProps.isVisible = hasValidCustomConfigVisibilityRules(context.customConfig)
-      ? plotProps.isVisible && baseRule
-      : baseRule;
     return plotProps;
   },
 });
 
 export const HistogramInfoHorizontal = connectToContainer(Info, {
   modifyPlotProps: (props, context, plotProps) => {
-    const baseRule =
+    plotProps.isVisible =
       context.fullContainer.type === 'histogram' && context.fullContainer.orientation === 'h';
-    plotProps.isVisible = hasValidCustomConfigVisibilityRules(context.customConfig)
-      ? plotProps.isVisible && baseRule
-      : baseRule;
     return plotProps;
   },
 });
 
 export const Histogram2d = connectToContainer(Info, {
   modifyPlotProps: (props, context, plotProps) => {
-    const baseRule = context.fullContainer.type === 'histogram2d';
-    plotProps.isVisible = hasValidCustomConfigVisibilityRules(context.customConfig)
-      ? plotProps.isVisible && baseRule
-      : baseRule;
+    plotProps.isVisible = context.fullContainer.type === 'histogram2d';
     return plotProps;
   },
 });
@@ -729,10 +708,7 @@ export const HovermodeDropdown = connectToContainer(UnconnectedVisibilitySelect,
 
 export const HoverColor = connectToContainer(UnconnectedColorPicker, {
   modifyPlotProps: (props, context, plotProps) => {
-    const baseRule = Boolean(context.fullLayout.hovermode);
-    plotProps.isVisible = hasValidCustomConfigVisibilityRules(context.customConfig)
-      ? plotProps.isVisible && baseRule
-      : baseRule;
+    plotProps.isVisible = Boolean(context.fullLayout.hovermode);
     return plotProps;
   },
 });
@@ -742,9 +718,7 @@ export const LevelRendered = connectToContainer(UnconnectedDropdown, {
     const _ = context.localize;
 
     if (context.container.ids && context.container.ids.length) {
-      plotProps.isVisible = hasValidCustomConfigVisibilityRules(context.customConfig)
-        ? plotProps.isVisible
-        : true;
+      plotProps.isVisible = true;
       plotProps.options = [{label: _('Root'), value: ''}].concat(
         context.container.ids.map(i => ({label: i, value: i}))
       );
diff --git a/src/lib/__tests__/unpackPlotProps-test.js b/src/lib/__tests__/unpackPlotProps-test.js
@@ -1,22 +1,11 @@
 import {computeCustomConfigVisibility} from '../index';
-import {Component} from 'react';
 
-class ColorscalePicker extends Component {
-  render() {
-    return null;
-  }
-}
-ColorscalePicker.displayName = 'ColorscalePicker';
-
-class AnotherPicker extends Component {
-  render() {
-    return null;
-  }
-}
-AnotherPicker.displayName = 'AnotherPicker';
-
-const validate = (string, expected, config, wrappedComponent) => {
-  const isVisible = computeCustomConfigVisibility({attr: string}, 'red', config, wrappedComponent);
+const validate = (string, expected, config, wrappedComponentDisplayName) => {
+  const isVisible = computeCustomConfigVisibility(
+    {attr: string},
+    config,
+    wrappedComponentDisplayName
+  );
   expect(isVisible).toBe(expected[string]);
 };
 
@@ -93,7 +82,7 @@ describe('computeCustomConfigVisibility', () => {
 
     const case1 = {'marker.color': false};
     const case2 = {'marker.color': true};
-    Object.keys(case1).forEach(c => validate(c, case1, config, ColorscalePicker));
-    Object.keys(case2).forEach(c => validate(c, case2, config, AnotherPicker));
+    Object.keys(case1).forEach(c => validate(c, case1, config, 'ColorscalePicker'));
+    Object.keys(case2).forEach(c => validate(c, case2, config, 'AnotherPicker'));
   });
 });
diff --git a/src/lib/connectToContainer.js b/src/lib/connectToContainer.js
@@ -1,6 +1,6 @@
 import React, {Component} from 'react';
 import PropTypes from 'prop-types';
-import unpackPlotProps from './unpackPlotProps';
+import unpackPlotProps, {validateVisibilityWithCustomConfig} from './unpackPlotProps';
 import {getDisplayName} from '../lib';
 
 export const containerConnectedContextTypes = {
@@ -63,7 +63,17 @@ export default function connectToContainer(WrappedComponent, config = {}) {
       // is also wrapped by a component that `unpackPlotProps`. That way inner
       // component can skip computation as it can see plotProps is already defined.
       const {plotProps = this.plotProps, ...props} = Object.assign({}, this.plotProps, this.props);
-      if (props.isVisible) {
+      const wrappedComponentDisplayName =
+        WrappedComponent && WrappedComponent.displayName ? WrappedComponent.displayName : null;
+
+      if (
+        validateVisibilityWithCustomConfig(
+          props.isVisible,
+          props,
+          this.context,
+          wrappedComponentDisplayName
+        )
+      ) {
         return <WrappedComponent {...props} plotProps={plotProps} />;
       }
 
diff --git a/src/lib/index.js b/src/lib/index.js
@@ -26,6 +26,7 @@ import tinyColor from 'tinycolor2';
 import unpackPlotProps, {
   computeCustomConfigVisibility,
   hasValidCustomConfigVisibilityRules,
+  validateVisibilityWithCustomConfig,
 } from './unpackPlotProps';
 import walkObject, {isPlainObject} from './walkObject';
 import {traceTypeToPlotlyInitFigure, plotlyTraceToCustomTrace} from './customTraceType';
@@ -265,5 +266,6 @@ export {
   transpose,
   unpackPlotProps,
   upperCase,
+  validateVisibilityWithCustomConfig,
   walkObject,
 };
diff --git a/src/lib/unpackPlotProps.js b/src/lib/unpackPlotProps.js
@@ -62,17 +62,11 @@ export function hasValidCustomConfigVisibilityRules(customConfig) {
   return false;
 }
 
-export function computeCustomConfigVisibility(props, fullValue, customConfig, wrappedComponent) {
+export function computeCustomConfigVisibility(props, customConfig, wrappedComponentDisplayName) {
   let isVisible;
 
   const isRegexMatch = rule => {
-    const stringToTest =
-      rule.type === 'attrName'
-        ? props.attr
-        : wrappedComponent && wrappedComponent.displayName
-        ? wrappedComponent.displayName
-        : '';
-
+    const stringToTest = rule.type === 'attrName' ? props.attr : wrappedComponentDisplayName;
     return RegExp(rule.regex_match).test(stringToTest);
   };
 
@@ -97,8 +91,21 @@ export function computeCustomConfigVisibility(props, fullValue, customConfig, wr
   return isVisible;
 }
 
-export default function unpackPlotProps(props, context, wrappedComponent) {
-  const {container, getValObject, defaultContainer, updateContainer, customConfig} = context;
+export function validateVisibilityWithCustomConfig(
+  initial,
+  nextProps,
+  nextContext,
+  componentDisplayName
+) {
+  let show = initial;
+  if (show && hasValidCustomConfigVisibilityRules(nextContext.customConfig)) {
+    show = computeCustomConfigVisibility(nextProps, nextContext.customConfig, componentDisplayName);
+  }
+  return show;
+}
+
+export default function unpackPlotProps(props, context) {
+  const {container, getValObject, defaultContainer, updateContainer} = context;
 
   if (!props.attr) {
     return {};
@@ -123,10 +130,7 @@ export default function unpackPlotProps(props, context, wrappedComponent) {
     multiValued = true;
   }
 
-  let isVisible = Boolean(hasFullValue(fullValue) || props.show);
-  if (isVisible && hasValidCustomConfigVisibilityRules(customConfig)) {
-    isVisible = computeCustomConfigVisibility(props, fullValue, customConfig, wrappedComponent);
-  }
+  const isVisible = Boolean(hasFullValue(fullValue) || props.show);
 
   let defaultValue = props.defaultValue;
   if (defaultValue === void 0 && defaultContainer) {
