From 5c78d6904ca8fade4fba4009a232b7ee8e3e8307 Mon Sep 17 00:00:00 2001
From: Nick Fitzpatrick <nick@metabase.com>
Date: Fri, 3 Mar 2023 19:00:35 -0400
Subject: [PATCH] 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
---
.../binning/binning-reproductions.cy.spec.js | 4 +-
e2e/test/scenarios/native/native.cy.spec.js | 2 +-
.../native/reproductions/16914.cy.spec.js | 2 +-
.../reproductions/17514-ui-overlay.cy.spec.js | 2 +-
.../scenarios/question/settings.cy.spec.js | 6 +-
.../visualizations/pivot_tables.cy.spec.js | 5 +-
...vot-settings-change-name-values.cy.spec.js | 5 +-
.../settings/ChartSettingFieldPicker.jsx | 13 ++--
.../ChartSettingFieldPicker.styled.tsx | 10 +++
.../settings/ChartSettingSelect.jsx | 8 ++-
.../settings/ChartSettingSelect.styled.tsx | 10 +++
.../components/settings/ColumnItem.jsx | 40 +++++++++--
.../components/settings/ColumnItem.styled.tsx | 8 +--
.../ChartSettingFieldsPicker.unit.spec.js | 71 +++++++++++++++++++
14 files changed, 158 insertions(+), 28 deletions(-)
create mode 100644 frontend/src/metabase/visualizations/components/settings/ChartSettingSelect.styled.tsx
create mode 100644 frontend/test/metabase/visualizations/components/settings/ChartSettingFieldsPicker.unit.spec.js
diff --git a/e2e/test/scenarios/binning/binning-reproductions.cy.spec.js b/e2e/test/scenarios/binning/binning-reproductions.cy.spec.js
index 8237092c2bf..238361f6152 100644
--- a/e2e/test/scenarios/binning/binning-reproductions.cy.spec.js
+++ b/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();
});
diff --git a/e2e/test/scenarios/native/native.cy.spec.js b/e2e/test/scenarios/native/native.cy.spec.js
index 21df66ea0a2..c24ece6030a 100644
--- a/e2e/test/scenarios/native/native.cy.spec.js
+++ b/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();
diff --git a/e2e/test/scenarios/native/reproductions/16914.cy.spec.js b/e2e/test/scenarios/native/reproductions/16914.cy.spec.js
index 2083d29e488..c0867b45928 100644
--- a/e2e/test/scenarios/native/reproductions/16914.cy.spec.js
+++ b/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();
diff --git a/e2e/test/scenarios/question/reproductions/17514-ui-overlay.cy.spec.js b/e2e/test/scenarios/question/reproductions/17514-ui-overlay.cy.spec.js
index 6406537b67e..0ca2f08448f 100644
--- a/e2e/test/scenarios/question/reproductions/17514-ui-overlay.cy.spec.js
+++ b/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();
});
}
diff --git a/e2e/test/scenarios/question/settings.cy.spec.js b/e2e/test/scenarios/question/settings.cy.spec.js
index 2a47216771f..85356c79385 100644
--- a/e2e/test/scenarios/question/settings.cy.spec.js
+++ b/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
diff --git a/e2e/test/scenarios/visualizations/pivot_tables.cy.spec.js b/e2e/test/scenarios/visualizations/pivot_tables.cy.spec.js
index b74144c695d..32cc4bd0c52 100644
--- a/e2e/test/scenarios/visualizations/pivot_tables.cy.spec.js
+++ b/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();
}
diff --git a/e2e/test/scenarios/visualizations/reproductions/15353-pivot-settings-change-name-values.cy.spec.js b/e2e/test/scenarios/visualizations/reproductions/15353-pivot-settings-change-name-values.cy.spec.js
index 96dca9f2acc..20949c73fd6 100644
--- a/e2e/test/scenarios/visualizations/reproductions/15353-pivot-settings-change-name-values.cy.spec.js
+++ b/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();
diff --git a/frontend/src/metabase/visualizations/components/settings/ChartSettingFieldPicker.jsx b/frontend/src/metabase/visualizations/components/settings/ChartSettingFieldPicker.jsx
index b65fe8531e4..150a0d3db29 100644
--- a/frontend/src/metabase/visualizations/components/settings/ChartSettingFieldPicker.jsx
+++ b/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}
/>
)}
diff --git a/frontend/src/metabase/visualizations/components/settings/ChartSettingFieldPicker.styled.tsx b/frontend/src/metabase/visualizations/components/settings/ChartSettingFieldPicker.styled.tsx
index bcba24c7685..644fd5b876c 100644
--- a/frontend/src/metabase/visualizations/components/settings/ChartSettingFieldPicker.styled.tsx
+++ b/frontend/src/metabase/visualizations/components/settings/ChartSettingFieldPicker.styled.tsx
@@ -1,6 +1,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 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")};
diff --git a/frontend/src/metabase/visualizations/components/settings/ChartSettingSelect.jsx b/frontend/src/metabase/visualizations/components/settings/ChartSettingSelect.jsx
index 5c12d6e1bb7..edb4b292481 100644
--- a/frontend/src/metabase/visualizations/components/settings/ChartSettingSelect.jsx
+++ b/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;
diff --git a/frontend/src/metabase/visualizations/components/settings/ChartSettingSelect.styled.tsx b/frontend/src/metabase/visualizations/components/settings/ChartSettingSelect.styled.tsx
new file mode 100644
index 00000000000..3a074a0175c
--- /dev/null
+++ b/frontend/src/metabase/visualizations/components/settings/ChartSettingSelect.styled.tsx
@@ -0,0 +1,10 @@
+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")};
+ }
+`;
diff --git a/frontend/src/metabase/visualizations/components/settings/ColumnItem.jsx b/frontend/src/metabase/visualizations/components/settings/ColumnItem.jsx
index 276e457af73..04dbf8a3137 100644
--- a/frontend/src/metabase/visualizations/components/settings/ColumnItem.jsx
+++ b/frontend/src/metabase/visualizations/components/settings/ColumnItem.jsx
@@ -1,6 +1,5 @@
/* 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>
diff --git a/frontend/src/metabase/visualizations/components/settings/ColumnItem.styled.tsx b/frontend/src/metabase/visualizations/components/settings/ColumnItem.styled.tsx
index 59f3e03c98c..9d1f6833e0d 100644
--- a/frontend/src/metabase/visualizations/components/settings/ColumnItem.styled.tsx
+++ b/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;
}
`;
diff --git a/frontend/test/metabase/visualizations/components/settings/ChartSettingFieldsPicker.unit.spec.js b/frontend/test/metabase/visualizations/components/settings/ChartSettingFieldsPicker.unit.spec.js
new file mode 100644
index 00000000000..45e58f7abc4
--- /dev/null
+++ b/frontend/test/metabase/visualizations/components/settings/ChartSettingFieldsPicker.unit.spec.js
@@ -0,0 +1,71 @@
+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"]);
+ });
+});
--
GitLab