Updating viz display picker, adding Visualization type (#27923)

* Updating viz display picker, adding Visualization type

* Adding Tests

* Removing title on Display picker sidebar

* PR Feedback

* adjusting e2e tests

frontend/src/metabase/query_builder/components/view/sidebars/ChartTypeSidebar.jsx

-/* eslint-disable react/prop-types */
-import React from "react";
-import _ from "underscore";
-import { t } from "ttag";
-import Icon from "metabase/components/Icon";
-import SidebarContent from "metabase/query_builder/components/SidebarContent";
-
-import visualizations from "metabase/visualizations";
-import {
-  OptionIconContainer,
-  OptionList,
-  OptionRoot,
-  OptionText,
-} from "./ChartTypeOption.styled";
-
-const FIXED_LAYOUT = [
-  ["line", "bar", "combo", "area", "row", "waterfall"],
-  ["scatter", "pie", "funnel", "smartscalar", "progress", "gauge"],
-  ["scalar", "table", "pivot", "map"],
-];
-const FIXED_TYPES = new Set(_.flatten(FIXED_LAYOUT));
-
-const ChartTypeSidebar = ({
-  question,
-  result,
-  onOpenChartSettings,
-  onCloseChartType,
-  updateQuestion,
-  isShowingChartTypeSidebar,
-  setUIControls,
-  ...props
-}) => {
-  const other = Array.from(visualizations)
-    .filter(
-      ([type, visualization]) =>
-        !visualization.hidden && !FIXED_TYPES.has(type),
-    )
-    .map(([type]) => type);
-  const otherGrouped = Object.values(
-    _.groupBy(other, (_, index) => Math.floor(index / 4)),
-  );
-
-  const layout = [...FIXED_LAYOUT, ...otherGrouped];
-
-  return (
-    <SidebarContent
-      className="full-height px1"
-      title={t`Choose a visualization`}
-      onDone={onCloseChartType}
-    >
-      {layout.map((row, index) => (
-        <OptionList key={index}>
-          {row.map(type => {
-            const visualization = visualizations.get(type);
-            return (
-              visualization && (
-                <ChartTypeOption
-                  key={type}
-                  visualization={visualization}
-                  isSelected={type === question.display()}
-                  isSensible={
-                    result &&
-                    result.data &&
-                    visualization.isSensible &&
-                    visualization.isSensible(result.data, props.query)
-                  }
-                  onClick={() => {
-                    const newQuestion = question
-                      .setDisplay(type)
-                      .lockDisplay(true); // prevent viz auto-selection
-
-                    updateQuestion(newQuestion, {
-                      reload: false,
-                      shouldUpdateUrl: question.query().isEditable(),
-                    });
-                    onOpenChartSettings({ section: t`Data` });
-                    setUIControls({ isShowingRawTable: false });
-                  }}
-                />
-              )
-            );
-          })}
-        </OptionList>
-      ))}
-    </SidebarContent>
-  );
-};
-
-const ChartTypeOption = ({
-  visualization,
-  isSelected,
-  isSensible,
-  onClick,
-}) => (
-  <OptionRoot isSensible={isSensible}>
-    <OptionIconContainer
-      isSelected={isSelected}
-      onClick={onClick}
-      data-testid={`${visualization.uiName}-button`}
-      data-is-sensible={isSensible}
-    >
-      <Icon name={visualization.iconName} size={20} />
-    </OptionIconContainer>
-    <OptionText>{visualization.uiName}</OptionText>
-  </OptionRoot>
-);
-
-export default ChartTypeSidebar;

frontend/src/metabase/query_builder/components/view/sidebars/ChartTypeOption.styled.tsx → frontend/src/metabase/query_builder/components/view/sidebars/ChartTypeSidebar.styled.tsx

@@ -2,7 +2,7 @@ import styled from "@emotion/styled";
 import { color, lighten, tint, isDark } from "metabase/lib/colors";
 
 export interface OptionRootProps {
-  isSensible?: boolean;
+  isSelected?: boolean;
 }
 
 const getOptionIconColor = ({ isSelected }: OptionIconContainerProps) => {
@@ -17,41 +17,63 @@ const getOptionIconColor = ({ isSelected }: OptionIconContainerProps) => {
 
 export const OptionRoot = styled.div<OptionRootProps>`
   padding: 0.5rem;
-  width: 33.33%;
-  opacity: ${props => (!props.isSensible ? 0.25 : 1)};
+  width: 25%;
   text-align: center;
+
+  ${props =>
+    props.isSelected &&
+    `
+    ${OptionIconContainer} {
+      background-color: ${color("brand")};
+      color: ${getOptionIconColor(props)};
+      border: 1px solid transparent;
+    }
+
+    ${OptionText} {
+      color: ${color("brand")};
+    }
+  `}
 `;
 
 export interface OptionIconContainerProps {
   isSelected?: boolean;
 }
 
+export const OptionText = styled.div`
+  margin-top: 0.5rem;
+  color: ${color("text-medium")};
+  font-weight: bold;
+  font-size: 0.75rem;
+`;
+
 export const OptionIconContainer = styled.div<OptionIconContainerProps>`
-  display: flex;
+  display: inline-flex;
   flex-direction: column;
   justify-content: center;
   align-items: center;
   color: ${getOptionIconColor};
-  background-color: ${props =>
-    props.isSelected ? color("brand") : lighten("brand")};
-  padding: 0.75rem;
-  border-radius: 0.625rem;
+  background-color: ${props => props.isSelected && color("brand")};
+  border-radius: 100%;
+  border: 1px solid ${color("border")};
   cursor: pointer;
-
+  padding: 0.875rem;
   &:hover {
     color: ${color("white")};
     background-color: ${color("brand")};
+    border: 1px solid transparent;
   }
 `;
 
-export const OptionText = styled.div`
-  margin-top: 0.5rem;
-  color: ${color("brand")};
+export const OptionLabel = styled.h4`
+  color: ${color("text-medium")};
   font-weight: bold;
+  font-size: 0.75rem;
+  text-transform: uppercase;
+  margin: 1rem 0 1rem 1.5rem;
 `;
 
 export const OptionList = styled.div`
   display: flex;
-  margin: 0 1rem 0.5rem 1rem;
+  margin: 1rem 1rem 3rem 1rem;
   flex-wrap: wrap;
 `;

frontend/src/metabase/query_builder/components/view/sidebars/ChartTypeSidebar.tsx

+import React, { useCallback, useMemo } from "react";
+import _ from "underscore";
+import { t } from "ttag";
+import Icon from "metabase/components/Icon";
+import SidebarContent from "metabase/query_builder/components/SidebarContent";
+
+import visualizations from "metabase/visualizations";
+import { Visualization } from "metabase/visualizations/shared/types/visualization";
+
+import Question from "metabase-lib/Question";
+import Query from "metabase-lib/queries/Query";
+
+import {
+  OptionIconContainer,
+  OptionList,
+  OptionRoot,
+  OptionText,
+  OptionLabel,
+} from "./ChartTypeSidebar.styled";
+
+const DEFAULT_ORDER = [
+  "table",
+  "bar",
+  "line",
+  "pie",
+  "scalar",
+  "row",
+  "area",
+  "combo",
+  "pivot",
+  "smartscalar",
+  "gauge",
+  "progress",
+  "funnel",
+  "object",
+  "map",
+  "scatter",
+  "waterfall",
+];
+
+interface ChartTypeSidebarProps {
+  question: Question;
+  result: any;
+  onOpenChartSettings: (props: { section: string }) => void;
+  onCloseChartType: () => void;
+  updateQuestion: (
+    question: Question,
+    props: { reload: boolean; shouldUpdateUrl: boolean },
+  ) => void;
+  setUIControls: (props: { isShowingRawTable: boolean }) => void;
+  query: Query;
+}
+
+const ChartTypeSidebar = ({
+  question,
+  result,
+  onOpenChartSettings,
+  onCloseChartType,
+  updateQuestion,
+  setUIControls,
+  query,
+}: ChartTypeSidebarProps) => {
+  const [makesSense, nonSense] = useMemo(() => {
+    return _.partition(
+      _.union(
+        DEFAULT_ORDER,
+        Array.from(visualizations)
+          .filter(([_type, visualization]) => !visualization.hidden)
+          .map(([vizType]) => vizType),
+      ),
+      vizType => {
+        const visualization = visualizations.get(vizType);
+        return (
+          result &&
+          result.data &&
+          visualization.isSensible &&
+          visualization.isSensible(result.data, query)
+        );
+      },
+    );
+  }, [result, query]);
+
+  const handleClick = useCallback(
+    display => {
+      const newQuestion = question.setDisplay(display).lockDisplay(); // prevent viz auto-selection
+
+      updateQuestion(newQuestion, {
+        reload: false,
+        shouldUpdateUrl: question.query().isEditable(),
+      });
+      onOpenChartSettings({ section: t`Data` });
+      setUIControls({ isShowingRawTable: false });
+    },
+    [question, updateQuestion, onOpenChartSettings, setUIControls],
+  );
+
+  return (
+    <SidebarContent className="full-height px1" onDone={onCloseChartType}>
+      <OptionList data-testid="display-options-sensible">
+        {makesSense.map(type => {
+          const visualization = visualizations.get(type);
+          return (
+            visualization && (
+              <ChartTypeOption
+                key={type}
+                visualization={visualization}
+                isSelected={type === question.display()}
+                isSensible
+                onClick={() => handleClick(type)}
+              />
+            )
+          );
+        })}
+      </OptionList>
+      <OptionLabel>{t`Other charts`}</OptionLabel>
+      <OptionList data-testid="display-options-not-sensible">
+        {nonSense.map(type => {
+          const visualization = visualizations.get(type);
+          return (
+            visualization && (
+              <ChartTypeOption
+                key={type}
+                visualization={visualization}
+                isSelected={type === question.display()}
+                isSensible={false}
+                onClick={() => handleClick(type)}
+              />
+            )
+          );
+        })}
+      </OptionList>
+    </SidebarContent>
+  );
+};
+
+interface ChartTypeOptionProps {
+  isSelected: boolean;
+  isSensible: boolean;
+  onClick: () => void;
+  visualization: Visualization;
+}
+
+const ChartTypeOption = ({
+  visualization,
+  isSelected,
+  isSensible,
+  onClick,
+}: ChartTypeOptionProps) => (
+  <OptionRoot
+    isSelected={isSelected}
+    data-testid={`${visualization.uiName}-container`}
+    role="option"
+    aria-selected={isSelected}
+  >
+    <OptionIconContainer
+      onClick={onClick}
+      data-is-sensible={isSensible}
+      data-testid={`${visualization.uiName}-button`}
+    >
+      <Icon name={visualization.iconName} size={20} />
+    </OptionIconContainer>
+    <OptionText>{visualization.uiName}</OptionText>
+  </OptionRoot>
+);
+
+export default ChartTypeSidebar;

frontend/src/metabase/visualizations/shared/types/visualization.ts

+import { DatasetData, Series, VisualizationSettings } from "metabase-types/api";
+
+export type Visualization = {
+  uiName: string;
+  identifier: string;
+  iconName: string;
+  noun: string;
+  hidden?: boolean;
+  noHeader: boolean;
+  minSize: {
+    width: number;
+    height: number;
+  };
+
+  isSensible: (data: DatasetData) => boolean;
+  isLiveResizable: (series: Series) => boolean;
+
+  settings: VisualizationSettings;
+
+  transformSeries: (series: Series) => Series;
+
+  checkRenderable: (series: Series, settings: VisualizationSettings) => boolean;
+
+  placeHolderSeries: Series;
+};

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

+import React from "react";
+import { render, fireEvent, screen, within } from "@testing-library/react";
+
+import { SAMPLE_DATABASE } from "__support__/sample_database_fixture";
+
+import ChartTypeSidebar from "metabase/query_builder/components/view/sidebars/ChartTypeSidebar";
+
+const DATA = {
+  rows: [[1]],
+  cols: [{ base_type: "type/Integer", name: "foo", display_name: "foo" }],
+};
+
+const setup = props => {
+  const question = SAMPLE_DATABASE.question().setDisplay("gauge");
+  render(
+    <ChartTypeSidebar
+      question={question}
+      query={question.query()}
+      result={{ data: DATA }}
+      {...props}
+    />,
+  );
+};
+
+describe("ChartSettingsSidebar", () => {
+  it("should highlight the correct display type", () => {
+    setup();
+
+    //active display type
+    expect(screen.getByRole("option", { selected: true })).toHaveTextContent(
+      "Gauge",
+    );
+  });
+
+  it("should call correct functions when display type is selected", () => {
+    const onOpenChartSettings = jest.fn();
+    const updateQuestion = jest.fn();
+    const setUIControls = jest.fn();
+
+    setup({
+      onOpenChartSettings,
+      updateQuestion,
+      setUIControls,
+    });
+
+    fireEvent.click(
+      within(screen.getByTestId("Progress-button")).getByRole("img"),
+    );
+
+    expect(onOpenChartSettings).toHaveBeenCalledWith({ section: "Data" });
+    expect(setUIControls).toHaveBeenCalledWith({ isShowingRawTable: false });
+    expect(updateQuestion).toHaveBeenCalled();
+  });
+
+  it("should group sensible and nonsensible options separately and in the correct order", () => {
+    setup();
+
+    const sensible = within(
+      screen.getByTestId("display-options-sensible"),
+    ).getAllByTestId(/container/i);
+    const nonSensible = within(
+      screen.getByTestId("display-options-not-sensible"),
+    ).getAllByTestId(/container/i);
+
+    expect(sensible).toHaveLength(5);
+    expect(nonSensible).toHaveLength(12);
+
+    const sensibleOrder = [
+      "Table",
+      "Number",
+      "Gauge",
+      "Progress",
+      "Object Detail",
+    ];
+    const nonSensibleOrder = [
+      "Bar",
+      "Line",
+      "Pie",
+      "Row",
+      "Area",
+      "Combo",
+      "Pivot Table",
+      "Trend",
+      "Funnel",
+      "Map",
+      "Scatter",
+      "Waterfall",
+    ];
+
+    sensible.forEach((node, index) => {
+      expect(node).toHaveTextContent(sensibleOrder[index]);
+    });
+
+    nonSensible.forEach((node, index) => {
+      expect(node).toHaveTextContent(nonSensibleOrder[index]);
+    });
+  });
+});

frontend/test/metabase/scenarios/permissions/reproductions/22447-illogical-UI-elements-for-nodata.cy.spec.js

@@ -16,7 +16,7 @@ describe("UI elements that make no sense for users without data permissions (met
     cy.findByText("Settings");
     cy.findByText("Visualization").click();
 
-    cy.findByTextEnsureVisible("Choose a visualization");
+    cy.findByTestId("display-options-sensible");
     cy.icon("line").click();
 
     cy.findByTextEnsureVisible("Line options");

frontend/test/metabase/scenarios/visualizations/maps.cy.spec.js

@@ -71,7 +71,7 @@ describe("scenarios > visualizations > maps", () => {
     cy.findByText("Visualization").closest(".Button").as("vizButton");
     cy.get("@vizButton").find(".Icon-pinmap");
     cy.get("@vizButton").click();
-    cy.findByText("Choose a visualization");
+    cy.findByTestId("display-options-sensible");
 
     cy.findByTestId("sidebar-left").as("vizSidebar");