diff --git a/e2e/test/scenarios/binning/binning-reproductions.cy.spec.js b/e2e/test/scenarios/binning/binning-reproductions.cy.spec.js
index 8237092c2bf78939a1ad7597a2ac4951af15a9f8..238361f6152e6a73f0c41772f05003df001a978c 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 21df66ea0a280c92f22f11729434a877aa5cede6..c24ece6030ae40f56eca0760949d7a248c9915c0 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 2083d29e488dc0613a4d0be08318fcb504a72dff..c0867b45928cbfa3f38fcc754f74e7df88aebbc5 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 6406537b67edc142aa1a518a8ab5191bd46fa4e7..0ca2f08448f3b3fb9c2036d19d8f4af25b794ef4 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 2a47216771f155fb7475c674a2d26b6aa8f8f896..85356c7938579f02dd9f3d0f438a3d21bc38a043 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 b74144c695da37461ba11acb5917d67d1cd0e641..32cc4bd0c52fc9cbc448db9cb66014efb5e36c10 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 96dca9f2acce191b83db396ac2ecfd96f9b82542..20949c73fd67005bdddbdcb9c595e2d4478fba2f 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 b65fe8531e4ccea487153d4b10acf7af243c5ec4..150a0d3db29901d88f0486929e4e0b8157f31fe6 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 bcba24c768573e653a35f8543ae838ea19b86b88..644fd5b876c9604c0beffd11d20f4b2cf8a13df8 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 5c12d6e1bb75659ca6be57f389c007e6a8f18ec3..edb4b292481435a42d84e270de896cde7e062d5d 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 0000000000000000000000000000000000000000..3a074a0175c730bb884efb36b2af315c07ad732f
--- /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 276e457af737222ece3de65cebe308faeae62249..04dbf8a313704e3581f405dc661182f0526ca327 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 59f3e03c98c5f14aff09e903de5252e1b45d0d79..9d1f6833e0d2c40043033281fdbeac8510538d4e 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 0000000000000000000000000000000000000000..45e58f7abc4c2196c4fb41a70a7aad07bd72656f
--- /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"]);
+ });
+});