Fix bug in chart column settings back behavior (#11833)

* fix bug in chart column settings back behavior

* rearrange a bit to fix potential issue with one section and multiple columns

* lint tests

* fix flow & test

* update logic

* fix test and lint

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

@@ -191,12 +191,20 @@ class ChartSettings extends Component {
       visibleWidgets = sections[currentSection] || [];
     }
 
+    // This checks whether the current section contains a column settings widget
+    // at the top level. If it does, we avoid hiding the section tabs and
+    // overriding the sidebar title.
+    const currentSectionHasColumnSettings = (
+      sections[currentSection] || []
+    ).some(widget => widget.id === "column_settings");
+
     const extraWidgetProps = {
       // NOTE: special props to support adding additional fields
       question: question,
       addField: addField,
       onShowWidget: this.handleShowWidget,
       onEndShowWidget: this.handleEndShowWidget,
+      currentSectionHasColumnSettings,
     };
 
     const sectionPicker = (
@@ -239,7 +247,9 @@ class ChartSettings extends Component {
       // hide the section picker if the only widget is column_settings
       !(
         visibleWidgets.length === 1 &&
-        visibleWidgets[0].id === "column_settings"
+        visibleWidgets[0].id === "column_settings" &&
+        // and this section doesn't doesn't have that as a direct child
+        !currentSectionHasColumnSettings
       );
 
     // default layout with visualization

frontend/src/metabase/visualizations/components/settings/ChartNestedSettingColumns.jsx

@@ -38,14 +38,20 @@ class ColumnWidgets extends React.Component {
   componentDidMount() {
     const {
       setSidebarPropsOverride,
-      onChangeEditingObject,
       object,
+      onEndShowWidget,
+      currentSectionHasColumnSettings,
     } = this.props;
 
-    if (setSidebarPropsOverride) {
+    // These two props (title and onBack) are overridden to display a column
+    // name instead of the visualization type when viewing a column's settings.
+    // If the column setting is directly within the section rather than an
+    // additional widget we drilled into, clicking back should still return us
+    // to the visualization list. In that case, we don't override these at all.
+    if (setSidebarPropsOverride && !currentSectionHasColumnSettings) {
       setSidebarPropsOverride({
         title: displayNameForColumn(object),
-        onBack: () => onChangeEditingObject(),
+        onBack: onEndShowWidget,
       });
     }
   }

frontend/test/metabase/query_builder/components/view/sidebars/ChartSettingsSidebar.unit.spec.js

 import React from "react";
 import "@testing-library/jest-dom/extend-expect";
-import { render, fireEvent } from "@testing-library/react";
+import { render, fireEvent, cleanup } from "@testing-library/react";
 
 import { SAMPLE_DATASET } from "__support__/sample_dataset_fixture";
 
 import ChartSettingsSidebar from "metabase/query_builder/components/view/sidebars/ChartSettingsSidebar";
 
 describe("ChartSettingsSidebar", () => {
+  const data = {
+    rows: [[1]],
+    cols: [{ base_type: "type/Integer", name: "foo", display_name: "foo" }],
+  };
+  afterEach(cleanup);
+
   it("should hide title and section picker when viewing column settings", () => {
-    const data = {
-      rows: [["bar"]],
-      cols: [{ base_type: "type/Text", name: "foo", display_name: "foo" }],
-    };
     const { container, getByText, queryByText } = render(
       <ChartSettingsSidebar
         question={SAMPLE_DATASET.question()}
@@ -24,4 +26,39 @@ describe("ChartSettingsSidebar", () => {
     expect(queryByText("Table options")).toBe(null);
     expect(queryByText("Conditional Formatting")).toBe(null);
   });
+
+  it("should not hide the title for gauge charts", () => {
+    const { getByText } = render(
+      <ChartSettingsSidebar
+        question={SAMPLE_DATASET.question().setDisplay("gauge")}
+        result={{ data }}
+      />,
+    );
+    // see options header with sections
+    getByText("Gauge options");
+    getByText("Formatting");
+    getByText("Display");
+
+    // click on formatting section
+    fireEvent.click(getByText("Formatting"));
+
+    // you see the formatting stuff
+    getByText("Style");
+    // but the sections and back title are unchanged
+    getByText("Gauge options");
+    getByText("Formatting");
+    getByText("Display");
+  });
+
+  it("should not hide the title for scalar charts", () => {
+    const { getByText } = render(
+      <ChartSettingsSidebar
+        question={SAMPLE_DATASET.question().setDisplay("scalar")}
+        result={{ data }}
+      />,
+    );
+    // see header with formatting fields
+    getByText("Number options");
+    getByText("Style");
+  });
 });

frontend/test/metabase/visualizations/components/ChartSettings.unit.spec.js

@@ -131,13 +131,21 @@ describe("ChartSettings", () => {
   });
 
   it("should not show the section picker if showing a column setting", () => {
+    const columnSettingsWidget = widget({
+      title: "Something",
+      section: "Formatting",
+      hidden: true,
+      id: "column_settings",
+    });
     const { queryByText } = render(
       <ChartSettings
         {...DEFAULT_PROPS}
         widgets={[
-          widget({ title: "Something", section: "Foo", id: "column_settings" }),
+          widget({ title: "List of columns", section: "Foo", id: "thing" }),
           widget({ title: "Other Thing", section: "Bar", id: "other_thing" }),
+          columnSettingsWidget,
         ]}
+        initial={{ widget: columnSettingsWidget }}
       />,
     );
     expect(queryByText("Foo")).toBe(null);