Standardize chart settings icon buttons (#28858)

* adding hover states, switching icons for buttons

* ChartSettingFieldsPicker unit tests

* fixing background hover state

* using userEvent.click instead of fireEvent

* adjusting e2e tests

e2e/test/scenarios/binning/binning-reproductions.cy.spec.js

@@ -207,7 +207,9 @@ describe("binning related reproductions", () => {
     cy.findByTestId("sidebar-left").within(() => {
       cy.findByTestId("Table-button").click();
       cy.findByTextEnsureVisible("Table options");
-      cy.findByText("Created At").siblings(".Icon-eye_outline").click();
+      cy.findByText("Created At")
+        .siblings("[data-testid$=hide-button]")
+        .click();
       cy.button("Done").click();
     });
 

e2e/test/scenarios/native/native.cy.spec.js

@@ -166,7 +166,7 @@ describe("scenarios > question > native", () => {
     cy.findByTestId("sidebar-left")
       .as("sidebar")
       .contains(/hidden/i)
-      .siblings(".Icon-eye_outline")
+      .siblings("[data-testid$=hide-button]")
       .click();
     cy.get("@editor").type("{movetoend}, 3 as added");
     cy.get("@runQuery").click();

e2e/test/scenarios/native/reproductions/16914.cy.spec.js

@@ -19,7 +19,7 @@ describe("issue 16914", () => {
     cy.findByTestId("viz-settings-button").click();
     cy.findByTestId("sidebar-left")
       .contains(/hidden/i)
-      .siblings(".Icon-eye_outline")
+      .siblings("[data-testid$=hide-button]")
       .click();
     cy.button("Done").click();
 

e2e/test/scenarios/question/reproductions/17514-ui-overlay.cy.spec.js

@@ -154,7 +154,7 @@ function openVisualizationOptions() {
 
 function hideColumn(columnName) {
   cy.findByTestId("chartsettings-sidebar").within(() => {
-    cy.findByText(columnName).siblings(".Icon-eye_outline").click();
+    cy.findByText(columnName).siblings("[data-testid$=hide-button]").click();
   });
 }
 

e2e/test/scenarios/question/settings.cy.spec.js

@@ -38,14 +38,14 @@ describe("scenarios > question > settings", () => {
       cy.get("@tableOptions")
         .contains("Total")
         .scrollIntoView()
-        .nextAll(".Icon-eye_outline")
+        .siblings("[data-testid$=hide-button]")
         .click();
 
       // Add people.category
       cy.get("@tableOptions")
         .contains("Category")
         .scrollIntoView()
-        .nextAll(".Icon-add")
+        .siblings("[data-testid$=add-button]")
         .click();
 
       // wait a Category value to appear in the table, so we know the query completed
@@ -55,7 +55,7 @@ describe("scenarios > question > settings", () => {
       cy.get("@tableOptions")
         .contains("Ean")
         .scrollIntoView()
-        .nextAll(".Icon-add")
+        .siblings("[data-testid$=add-button]")
         .click();
 
       // wait a Ean value to appear in the table, so we know the query completed

e2e/test/scenarios/visualizations/pivot_tables.cy.spec.js

@@ -1044,5 +1044,8 @@ function getIframeBody(selector = "iframe") {
 }
 
 function openColumnSettings(columnName) {
-  sidebar().findByText(columnName).siblings(".Icon-ellipsis").click();
+  sidebar()
+    .findByText(columnName)
+    .siblings("[data-testid$=settings-button]")
+    .click();
 }

e2e/test/scenarios/visualizations/reproductions/15353-pivot-settings-change-name-values.cy.spec.js

@@ -25,7 +25,10 @@ describe("issue 15353", () => {
 
   it("should be able to change field name used for values (metabase#15353)", () => {
     cy.findByTestId("viz-settings-button").click();
-    sidebar().contains("Count").siblings(".Icon-ellipsis").click();
+    sidebar()
+      .contains("Count")
+      .siblings("[data-testid$=settings-button]")
+      .click();
 
     cy.findByDisplayValue("Count").type(" renamed").blur();
 

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

@@ -7,6 +7,7 @@ import { getColumnKey } from "metabase-lib/queries/utils/get-column-key";
 import ChartSettingSelect from "./ChartSettingSelect";
 import {
   SettingsIcon,
+  SettingsButton,
   ChartSettingFieldPickerRoot,
   FieldPickerColorPicker,
 } from "./ChartSettingFieldPicker.styled";
@@ -73,8 +74,9 @@ const ChartSettingFieldPicker = ({
         hiddenIcons
       />
       {columnKey && (
-        <SettingsIcon
-          name="ellipsis"
+        <SettingsButton
+          onlyIcon
+          icon="ellipsis"
           onClick={e => {
             onShowWidget(
               {
@@ -89,10 +91,11 @@ const ChartSettingFieldPicker = ({
         />
       )}
       {onRemove && (
-        <SettingsIcon
+        <SettingsButton
           data-testid={`remove-${value}`}
-          name="close"
-          size={14}
+          icon="close"
+          iconSize={14}
+          onlyIcon
           onClick={onRemove}
         />
       )}

frontend/src/metabase/visualizations/components/settings/ChartSettingFieldPicker.styled.tsx

 import styled from "@emotion/styled";
 import { color } from "metabase/lib/colors";
 import Icon from "metabase/components/Icon";
+import Button from "metabase/core/components/Button";
 import SelectButton from "metabase/core/components/SelectButton";
 import Triggerable from "metabase/components/Triggerable";
 import ChartSettingColorPicker from "./ChartSettingColorPicker";
@@ -57,6 +58,15 @@ interface SettingsIconProps {
   noMargin?: boolean;
 }
 
+export const SettingsButton = styled(Button)<SettingsIconProps>`
+  margin-left: ${props => (props.noMargin ? "0" : "0.75rem")};
+  padding: 0;
+
+  &:hover {
+    background-color: unset;
+  }
+`;
+
 export const SettingsIcon = styled(Icon)<SettingsIconProps>`
   margin-left: ${props => (props.noMargin ? "0" : "0.75rem")};
   color: ${color("text-medium")};

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

@@ -2,7 +2,9 @@
 import React from "react";
 
 import cx from "classnames";
-import Select, { Option } from "metabase/core/components/Select";
+import { Option } from "metabase/core/components/Select";
+
+import { SelectWithHighlightingIcon } from "./ChartSettingSelect.styled";
 
 const ChartSettingSelect = ({
   // Use null if value is undefined. If we pass undefined, Select will create an
@@ -16,7 +18,7 @@ const ChartSettingSelect = ({
   placeholderNoOptions,
   ...props
 }) => (
-  <Select
+  <SelectWithHighlightingIcon
     className={cx(className, "block")}
     disabled={
       options.length === 0 ||
@@ -33,7 +35,7 @@ const ChartSettingSelect = ({
         {option.name}
       </Option>
     ))}
-  </Select>
+  </SelectWithHighlightingIcon>
 );
 
 export default ChartSettingSelect;

frontend/src/metabase/visualizations/components/settings/ChartSettingSelect.styled.tsx

+import styled from "@emotion/styled";
+import Select from "metabase/core/components/Select";
+import SelectButton from "metabase/core/components/SelectButton";
+import { color } from "metabase/lib/colors";
+
+export const SelectWithHighlightingIcon = styled(Select)`
+  ${SelectButton.Icon}:hover {
+    color: ${color("brand")};
+  }
+`;

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

 /* eslint-disable react/prop-types */
 import React from "react";
-
 import {
   ColumnItemIcon,
   ColumnItemSpan,
@@ -11,9 +10,12 @@ import {
   ColumnItemColorPicker,
 } from "./ColumnItem.styled";
 
-const ActionIcon = ({ icon, onClick }) => (
+const ActionIcon = ({ icon, onClick, ...props }) => (
   <ColumnItemIcon
-    name={icon}
+    data-testid={props["data-testid"]}
+    onlyIcon
+    icon={icon}
+    iconSize={16}
     onClick={e => {
       e.stopPropagation();
       onClick(e.target);
@@ -51,10 +53,34 @@ const ColumnItem = ({
         )}
         <ColumnItemContent>
           <ColumnItemSpan>{title}</ColumnItemSpan>
-          {onEdit && <ActionIcon icon="ellipsis" onClick={onEdit} />}
-          {onAdd && <ActionIcon icon="add" onClick={onAdd} />}
-          {onRemove && <ActionIcon icon="eye_outline" onClick={onRemove} />}
-          {onEnable && <ActionIcon icon="eye_crossed_out" onClick={onEnable} />}
+          {onEdit && (
+            <ActionIcon
+              icon="ellipsis"
+              onClick={onEdit}
+              data-testid={`${title}-settings-button`}
+            />
+          )}
+          {onAdd && (
+            <ActionIcon
+              icon="add"
+              onClick={onAdd}
+              data-testid={`${title}-add-button`}
+            />
+          )}
+          {onRemove && (
+            <ActionIcon
+              icon="eye_outline"
+              onClick={onRemove}
+              data-testid={`${title}-hide-button`}
+            />
+          )}
+          {onEnable && (
+            <ActionIcon
+              icon="eye_crossed_out"
+              onClick={onEnable}
+              data-testid={`${title}-show-button`}
+            />
+          )}
         </ColumnItemContent>
       </ColumnItemContainer>
     </ColumnItemRoot>

frontend/src/metabase/visualizations/components/settings/ColumnItem.styled.tsx

@@ -2,6 +2,7 @@ import styled from "@emotion/styled";
 import { color } from "metabase/lib/colors";
 
 import Icon from "metabase/components/Icon";
+import Button from "metabase/core/components/Button";
 import ChartSettingColorPicker from "./ChartSettingColorPicker";
 
 interface ColumnItemRootProps {
@@ -62,13 +63,12 @@ export const ColumnItemContainer = styled.div`
   align-items: center;
 `;
 
-export const ColumnItemIcon = styled(Icon)`
+export const ColumnItemIcon = styled(Button)`
   margin-left: 1rem;
-  cursor: pointer;
-  color: ${color("text-dark")};
+  padding: 0;
 
   &:hover {
-    color: ${color("text-medium")};
+    background-color: unset;
   }
 `;
 

frontend/test/metabase/visualizations/components/settings/ChartSettingFieldsPicker.unit.spec.js

+import React from "react";
+import userEvent from "@testing-library/user-event";
+import { renderWithProviders, screen } from "__support__/ui";
+
+import ChartSettingFieldsPicker from "metabase/visualizations/components/settings/ChartSettingFieldsPicker";
+
+const DEFAULT_PROPS = {
+  options: [
+    { name: "Count", value: "count" },
+    { name: "Average of Total", value: "avg" },
+  ],
+  colors: { avg: "#A989C5", count: "#509EE3" },
+  columnHasSettings: () => true,
+  value: ["avg", "count"],
+  addAnother: "Add another series",
+};
+
+const setup = props => {
+  renderWithProviders(
+    <ChartSettingFieldsPicker {...DEFAULT_PROPS} {...props} />,
+  );
+};
+
+describe("ChartSettingFieldsPicker", () => {
+  it("Should render both options", () => {
+    setup();
+
+    expect(screen.getByText("Average of Total")).toBeInTheDocument();
+    expect(screen.getByText("Count")).toBeInTheDocument();
+
+    //Expect there to be remove icons for both
+    expect(screen.getByTestId("remove-avg")).toBeInTheDocument();
+    expect(screen.getByTestId("remove-count")).toBeInTheDocument();
+  });
+
+  it("should show you a button to add another metric if there are unused options", () => {
+    const onChange = jest.fn();
+
+    setup({ value: ["avg"], onChange });
+
+    expect(screen.getByText("Average of Total")).toBeInTheDocument();
+    expect(screen.queryByTestId("remove-avg")).not.toBeInTheDocument();
+    expect(screen.getByText("Add another series")).toBeInTheDocument();
+
+    userEvent.click(screen.getByText("Add another series"));
+
+    expect(onChange).toHaveBeenCalledWith(["avg", "count"]);
+  });
+
+  it("should allow you to change an existing metric if there are unused options", () => {
+    const onChange = jest.fn();
+
+    setup({ value: ["avg"], onChange });
+
+    expect(screen.getByText("Average of Total")).toBeInTheDocument();
+    expect(screen.queryByTestId("remove-avg")).not.toBeInTheDocument();
+    expect(screen.getByText("Add another series")).toBeInTheDocument();
+    expect(
+      screen.getByRole("img", { name: /chevrondown/i }),
+    ).toBeInTheDocument();
+
+    userEvent.click(screen.getByText("Average of Total"));
+
+    //Check to see that count is in the popover
+    expect(screen.getByText("Count")).toBeInTheDocument();
+
+    userEvent.click(screen.getByText("Count"));
+
+    expect(onChange).toHaveBeenCalledWith(["count"]);
+  });
+});