Merge pull request #8789 from metabase/chart-setting-fixes

Chart setting fixes

frontend/src/metabase/query_builder/components/QueryVisualization.jsx

@@ -250,7 +250,7 @@ export default class QueryVisualization extends Component {
           <VisualizationResult
             lastRunDatasetQuery={this.state.lastRunDatasetQuery}
             onUpdateWarnings={warnings => this.setState({ warnings })}
-            onOpenChartSettings={() => this.refs.settings.open()}
+            onOpenChartSettings={initial => this.refs.settings.open(initial)}
             {...this.props}
             className="spread"
           />

frontend/src/metabase/query_builder/components/VisualizationSettings.jsx

@@ -90,8 +90,8 @@ export default class VisualizationSettings extends React.Component {
     );
   }
 
-  open = () => {
-    this.props.showChartSettings({});
+  open = initial => {
+    this.props.showChartSettings(initial || {});
   };
 
   close = () => {
@@ -123,7 +123,7 @@ export default class VisualizationSettings extends React.Component {
               ]}
               onChange={this.props.onReplaceAllVisualizationSettings}
               onClose={this.close}
-              initialWidget={chartSettings && chartSettings.widget}
+              initial={chartSettings}
             />
           </Modal>
         </div>

frontend/src/metabase/visualizations/components/ChartSettings.jsx

@@ -19,14 +19,15 @@ import {
 } from "metabase/visualizations";
 import { updateSettings } from "metabase/visualizations/lib/settings";
 
-const DEFAULT_TAB_PRIORITY = ["Display"];
+// section names are localized
+const DEFAULT_TAB_PRIORITY = [t`Display`];
 
 class ChartSettings extends Component {
   constructor(props) {
     super(props);
     this.state = {
-      currentTab: null,
-      showWidget: props.initialWidget,
+      currentSection: (props.initial && props.initial.section) || null,
+      currentWidget: (props.initial && props.initial.widget) || null,
       ...this._getState(
         props.series,
         props.series[0].card.visualization_settings,
@@ -56,8 +57,18 @@ class ChartSettings extends Component {
     };
   }
 
-  handleSelectTab = tab => {
-    this.setState({ currentTab: tab, showWidget: null });
+  handleShowSection = section => {
+    this.setState({ currentSection: section, currentWidget: null });
+  };
+
+  // allows a widget to temporarily replace itself with a different widget
+  handleShowWidget = widget => {
+    this.setState({ currentWidget: widget });
+  };
+
+  // go back to previously selected section
+  handleEndShowWidget = () => {
+    this.setState({ currentWidget: null });
   };
 
   handleResetSettings = () => {
@@ -79,21 +90,13 @@ class ChartSettings extends Component {
     this.props.onClose();
   };
 
-  // allows a widget to temporarily replace itself with a different widget
-  handleShowWidget = widget => {
-    this.setState({ showWidget: widget });
-  };
-  handleEndShowWidget = () => {
-    this.setState({ showWidget: null });
-  };
-
   render() {
     const { isDashboard, question, addField } = this.props;
-    const { rawSeries, transformedSeries, showWidget } = this.state;
+    const { rawSeries, transformedSeries, currentWidget } = this.state;
 
     const widgetsById = {};
 
-    const tabs = {};
+    const sections = {};
     for (const widget of getSettingsWidgetsForSeries(
       transformedSeries,
       this.handleChangeSettings,
@@ -101,38 +104,38 @@ class ChartSettings extends Component {
     )) {
       widgetsById[widget.id] = widget;
       if (widget.widget && !widget.hidden) {
-        tabs[widget.section] = tabs[widget.section] || [];
-        tabs[widget.section].push(widget);
+        sections[widget.section] = sections[widget.section] || [];
+        sections[widget.section].push(widget);
       }
     }
 
     // Move settings from the "undefined" section in the first tab
-    if (tabs["undefined"] && Object.values(tabs).length > 1) {
-      let extra = tabs["undefined"];
-      delete tabs["undefined"];
-      Object.values(tabs)[0].unshift(...extra);
+    if (sections["undefined"] && Object.values(sections).length > 1) {
+      let extra = sections["undefined"];
+      delete sections["undefined"];
+      Object.values(sections)[0].unshift(...extra);
     }
 
-    const tabNames = Object.keys(tabs);
-    const currentTab =
-      this.state.currentTab ||
-      _.find(DEFAULT_TAB_PRIORITY, name => name in tabs) ||
-      tabNames[0];
+    const sectionNames = Object.keys(sections);
+    const currentSection =
+      this.state.currentSection ||
+      _.find(DEFAULT_TAB_PRIORITY, name => name in sections) ||
+      sectionNames[0];
 
     let widgets;
-    let widget = showWidget && widgetsById[showWidget.id];
+    let widget = currentWidget && widgetsById[currentWidget.id];
     if (widget) {
       widget = {
         ...widget,
         hidden: false,
         props: {
           ...(widget.props || {}),
-          ...(showWidget.props || {}),
+          ...(currentWidget.props || {}),
         },
       };
       widgets = [widget];
     } else {
-      widgets = tabs[currentTab];
+      widgets = sections[currentSection];
     }
 
     const extraWidgetProps = {
@@ -145,12 +148,12 @@ class ChartSettings extends Component {
 
     return (
       <div className="flex flex-column spread">
-        {tabNames.length > 1 && (
+        {sectionNames.length > 1 && (
           <div className="border-bottom flex flex-no-shrink pl4">
             <Radio
-              value={currentTab}
-              onChange={this.handleSelectTab}
-              options={tabNames}
+              value={currentSection}
+              onChange={this.handleShowSection}
+              options={sectionNames}
               optionNameFn={v => v}
               optionValueFn={v => v}
               underlined

frontend/src/metabase/visualizations/components/LineAreaBarChart.jsx

@@ -99,7 +99,7 @@ export default class LineAreaBarChart extends Component {
     if (dimensions.length < 1 || metrics.length < 1) {
       throw new ChartSettingsError(
         t`Which fields do you want to use for the X and Y axes?`,
-        t`Data`,
+        { section: t`Data` },
         t`Choose fields`,
       );
     }

frontend/src/metabase/visualizations/components/Visualization.jsx

@@ -85,7 +85,7 @@ type Props = {
 
   // misc
   onUpdateWarnings: (string[]) => void,
-  onOpenChartSettings: () => void,
+  onOpenChartSettings: ({ section?: ?string, widget?: ?any }) => void,
 
   // number of grid cells wide and tall
   gridSize?: { width: number, height: number },
@@ -367,7 +367,7 @@ export default class Visualization extends Component {
                 <div className="mt2">
                   <button
                     className="Button Button--primary Button--medium"
-                    onClick={this.props.onOpenChartSettings}
+                    onClick={() => this.props.onOpenChartSettings(e.initial)}
                   >
                     {e.buttonText}
                   </button>

frontend/src/metabase/visualizations/lib/errors.js

@@ -3,6 +3,8 @@
 import { t, ngettext, msgid } from "c-3po";
 // NOTE: extending Error with Babel requires babel-plugin-transform-builtin-extend
 
+type ChartSettingsInitial = { section?: ?string, widget?: ?any };
+
 export class MinColumnsError extends Error {
   constructor(minColumns: number, actualColumns: number) {
     super(
@@ -42,11 +44,16 @@ export class NoBreakoutError extends Error {
 }
 
 export class ChartSettingsError extends Error {
-  section: ?string;
+  initial: ?ChartSettingsInitial;
   buttonText: ?string;
-  constructor(message: string, section?: string, buttonText?: string) {
+
+  constructor(
+    message: string,
+    initial?: ChartSettingsInitial,
+    buttonText?: string,
+  ) {
     super(message || t`Please configure this chart in the chart settings`);
-    this.section = section;
+    this.initial = initial;
     this.buttonText = buttonText || t`Edit Settings`;
   }
 }

frontend/src/metabase/visualizations/visualizations/Funnel.jsx

@@ -56,7 +56,7 @@ export default class Funnel extends Component {
     if (!settings["funnel.dimension"] || !settings["funnel.metric"]) {
       throw new ChartSettingsError(
         t`Which fields do you want to use?`,
-        t`Data`,
+        { section: t`Data` },
         t`Choose fields`,
       );
     }

frontend/src/metabase/visualizations/visualizations/Map.jsx

@@ -223,17 +223,19 @@ export default class Map extends Component {
       ) {
         throw new ChartSettingsError(
           t`Please select longitude and latitude columns in the chart settings.`,
-          "Data",
+          { section: t`Data` },
         );
       }
     } else if (settings["map.type"] === "region") {
       if (!settings["map.region"]) {
-        throw new ChartSettingsError(t`Please select a region map.`, "Data");
+        throw new ChartSettingsError(t`Please select a region map.`, {
+          section: t`Data`,
+        });
       }
       if (!settings["map.dimension"] || !settings["map.metric"]) {
         throw new ChartSettingsError(
           t`Please select region and metric columns in the chart settings.`,
-          "Data",
+          { section: t`Data` },
         );
       }
     }

frontend/src/metabase/visualizations/visualizations/PieChart.jsx

@@ -50,10 +50,9 @@ export default class PieChart extends Component {
 
   static checkRenderable([{ data: { cols, rows } }], settings) {
     if (!settings["pie.dimension"] || !settings["pie.metric"]) {
-      throw new ChartSettingsError(
-        t`Which columns do you want to use?`,
-        t`Data`,
-      );
+      throw new ChartSettingsError(t`Which columns do you want to use?`, {
+        section: `Data`,
+      });
     }
   }
 
@@ -91,10 +90,13 @@ export default class PieChart extends Component {
       title: t`Colors`,
       widget: "colors",
       getDefault: (series, settings) =>
-        getColorsForValues(settings["pie._dimensionValues"]),
+        settings["pie._dimensionValues"]
+          ? getColorsForValues(settings["pie._dimensionValues"])
+          : [],
       getProps: (series, settings) => ({
-        seriesTitles: settings["pie._dimensionValues"],
+        seriesTitles: settings["pie._dimensionValues"] || [],
       }),
+      getDisabled: (series, settings) => !settings["pie._dimensionValues"],
       readDependencies: ["pie._dimensionValues"],
     },
     // this setting recomputes color assignment using pie.colors as the existing
@@ -122,7 +124,10 @@ export default class PieChart extends Component {
     "pie._dimensionValues": {
       getValue: ([{ data: { rows } }], settings) => {
         const dimensionIndex = settings["pie._dimensionIndex"];
-        return rows.map(row => row[dimensionIndex]);
+        return dimensionIndex >= 0
+          ? // cast to string because getColorsForValues expects strings
+            rows.map(row => String(row[dimensionIndex]))
+          : null;
       },
       readDependencies: ["pie._dimensionIndex"],
     },