From b96a477f0bcb4c7b0982f7d303dd46b985b58da5 Mon Sep 17 00:00:00 2001
From: Anton Kulyk <kuliks.anton@gmail.com>
Date: Mon, 12 Jul 2021 19:21:41 +0300
Subject: [PATCH] Fix console errors for Radio and SegmentedControl (#16950)
---
.../components/PermissionsTabs.jsx | 2 +-
.../src/metabase/components/Radio.info.js | 29 ++++---
frontend/src/metabase/components/Radio.jsx | 85 ++++++++++++-------
.../src/metabase/components/Radio.styled.js | 31 ++++++-
.../metabase/components/SegmentedControl.jsx | 48 ++++++-----
.../components/SegmentedControl.styled.js | 16 +++-
frontend/src/metabase/css/components/form.css | 22 -----
.../components/ChartSettings.jsx | 1 +
.../components/ChartSettings.unit.spec.js | 26 +++---
9 files changed, 160 insertions(+), 100 deletions(-)
diff --git a/frontend/src/metabase/admin/permissions/components/PermissionsTabs.jsx b/frontend/src/metabase/admin/permissions/components/PermissionsTabs.jsx
index 6b313b63ac4..40cb5accc8b 100644
--- a/frontend/src/metabase/admin/permissions/components/PermissionsTabs.jsx
+++ b/frontend/src/metabase/admin/permissions/components/PermissionsTabs.jsx
@@ -13,7 +13,7 @@ const PermissionsTabs = ({ tab, onChangeTab }) => (
{ name: t`Data permissions`, value: `databases` },
{ name: t`Collection permissions`, value: `collections` },
]}
- onChange={onChangeTab}
+ onOptionClick={onChangeTab}
variant="underlined"
py={2}
/>
diff --git a/frontend/src/metabase/components/Radio.info.js b/frontend/src/metabase/components/Radio.info.js
index b5fb63b2712..afdcff56ded 100644
--- a/frontend/src/metabase/components/Radio.info.js
+++ b/frontend/src/metabase/components/Radio.info.js
@@ -1,4 +1,4 @@
-import React from "react";
+import React, { useState } from "react";
import Radio from "metabase/components/Radio";
export const component = Radio;
@@ -8,15 +8,24 @@ export const description = `
A standard radio button group.
`;
-const PROPS = {
- value: 0,
- options: [{ name: "Gadget", value: 0 }, { name: "Gizmo", value: 1 }],
-};
+const OPTIONS = [{ name: "Gadget", value: 0 }, { name: "Gizmo", value: 1 }];
+
+function RadioDemo(props) {
+ const [value, setValue] = useState(0);
+ return (
+ <Radio
+ {...props}
+ options={OPTIONS}
+ value={value}
+ onChange={nextValue => setValue(nextValue)}
+ />
+ );
+}
export const examples = {
- default: <Radio {...PROPS} />,
- underlined: <Radio {...PROPS} variant="underlined" />,
- "show buttons": <Radio {...PROPS} showButtons />,
- vertical: <Radio {...PROPS} vertical />,
- bubble: <Radio {...PROPS} variant="bubble" />,
+ default: <RadioDemo />,
+ underlined: <RadioDemo variant="underlined" />,
+ "show buttons": <RadioDemo showButtons />,
+ vertical: <RadioDemo vertical />,
+ bubble: <RadioDemo variant="bubble" />,
};
diff --git a/frontend/src/metabase/components/Radio.jsx b/frontend/src/metabase/components/Radio.jsx
index 56041536f5f..2a0dcff09be 100644
--- a/frontend/src/metabase/components/Radio.jsx
+++ b/frontend/src/metabase/components/Radio.jsx
@@ -4,6 +4,8 @@ import _ from "underscore";
import Icon from "metabase/components/Icon";
import {
+ RadioInput,
+ RadioButton,
BubbleList,
BubbleItem,
NormalList,
@@ -25,8 +27,12 @@ const optionShape = PropTypes.shape({
const propTypes = {
name: PropTypes.string,
value: PropTypes.any,
- options: PropTypes.arrayOf(optionShape).isRequired,
+ options: PropTypes.oneOfType([
+ PropTypes.arrayOf(PropTypes.string),
+ PropTypes.arrayOf(optionShape),
+ ]).isRequired,
onChange: PropTypes.func,
+ onOptionClick: PropTypes.func,
optionNameFn: PropTypes.func,
optionValueFn: PropTypes.func,
optionKeyFn: PropTypes.func,
@@ -51,9 +57,15 @@ const VARIANTS = {
function Radio({
name: nameFromProps,
- value,
+ value: currentValue,
options,
+
+ // onChange won't fire when you click an already checked item
+ // onOptionClick will fire in any case
+ // onOptionClick can be used for e.g. tab navigation like on the admin Permissions page)
+ onOptionClick,
onChange,
+
optionNameFn = defaultNameGetter,
optionValueFn = defaultValueGetter,
optionKeyFn = defaultValueGetter,
@@ -70,7 +82,7 @@ function Radio({
const [List, Item] = VARIANTS[variant] || VARIANTS.normal;
- if (variant === "underlined" && value === undefined) {
+ if (variant === "underlined" && currentValue === undefined) {
console.warn(
"Radio can't underline selected option when no value is given.",
);
@@ -79,35 +91,48 @@ function Radio({
return (
<List {...props} vertical={vertical} showButtons={showButtons}>
{options.map((option, index) => {
- const selected = value === optionValueFn(option);
+ const value = optionValueFn(option);
+ const selected = currentValue === value;
const last = index === options.length - 1;
+ const key = optionKeyFn(option);
+ const id = `${name}-${key}`;
+ const labelId = `${id}-label`;
return (
- <Item
- key={optionKeyFn(option)}
- selected={selected}
- last={last}
- vertical={vertical}
- showButtons={showButtons}
- py={py}
- xspace={xspace}
- yspace={yspace}
- onClick={e => onChange(optionValueFn(option))}
- aria-selected={selected}
- >
- {option.icon && <Icon name={option.icon} mr={1} />}
- <input
- className="Form-radio"
- type="radio"
- name={name}
- value={optionValueFn(option)}
- checked={selected}
- id={name + "-" + optionKeyFn(option)}
- />
- {showButtons && (
- <label htmlFor={name + "-" + optionKeyFn(option)} />
- )}
- <span>{optionNameFn(option)}</span>
- </Item>
+ <li key={key}>
+ <Item
+ id={labelId}
+ htmlFor={id}
+ selected={selected}
+ last={last}
+ vertical={vertical}
+ showButtons={showButtons}
+ py={py}
+ xspace={xspace}
+ yspace={yspace}
+ onClick={() => {
+ if (typeof onOptionClick === "function") {
+ onOptionClick(value);
+ }
+ }}
+ >
+ {option.icon && <Icon name={option.icon} mr={1} />}
+ <RadioInput
+ id={id}
+ name={name}
+ value={value}
+ checked={selected}
+ onChange={() => {
+ if (typeof onChange === "function") {
+ onChange(value);
+ }
+ }}
+ // Workaround for https://github.com/testing-library/dom-testing-library/issues/877
+ aria-labelledby={labelId}
+ />
+ {showButtons && <RadioButton checked={selected} />}
+ {optionNameFn(option)}
+ </Item>
+ </li>
);
})}
</List>
diff --git a/frontend/src/metabase/components/Radio.styled.js b/frontend/src/metabase/components/Radio.styled.js
index aaabf848a0e..c7189d83d43 100644
--- a/frontend/src/metabase/components/Radio.styled.js
+++ b/frontend/src/metabase/components/Radio.styled.js
@@ -4,17 +4,46 @@ import { space } from "styled-system";
import { color, lighten } from "metabase/lib/colors";
+export const RadioInput = styled.input.attrs({ type: "radio" })`
+ cursor: inherit;
+ position: absolute;
+ opacity: 0;
+ width: 0;
+ height: 0;
+ top: 0;
+ left: 0;
+ margin: 0;
+ padding: 0;
+ z-index: 1;
+`;
+
+export const RadioButton = styled.div`
+ cursor: pointer;
+ display: inline-block;
+ flex: 0 0 auto;
+ position: relative;
+ margin-right: 0.5rem;
+ width: 12px;
+ height: 12px;
+ border: 2px solid white;
+ box-shadow: 0 0 0 2px ${color("shadow")};
+ border-radius: 12px;
+ background-color: ${props =>
+ props.checked ? color("brand") : "transparent"};
+`;
+
// BASE
const BaseList = styled.ul`
display: flex;
flex-direction: ${props => (props.vertical ? "column" : "row")};
`;
-const BaseItem = styled.li.attrs({
+const BaseItem = styled.label.attrs({
mr: props => (!props.vertical && !props.last ? props.xspace : null),
mb: props => (props.vertical && !props.last ? props.yspace : null),
})`
${space}
+ position: relative;
display: flex;
align-items: center;
cursor: pointer;
diff --git a/frontend/src/metabase/components/SegmentedControl.jsx b/frontend/src/metabase/components/SegmentedControl.jsx
index 61ac026eb51..1d42d47d7fd 100644
--- a/frontend/src/metabase/components/SegmentedControl.jsx
+++ b/frontend/src/metabase/components/SegmentedControl.jsx
@@ -2,7 +2,11 @@ import React, { useMemo } from "react";
import PropTypes from "prop-types";
import _ from "underscore";
import Icon from "metabase/components/Icon";
-import { SegmentedList, SegmentedItem } from "./SegmentedControl.styled";
+import {
+ SegmentedList,
+ SegmentedItem,
+ SegmentedControlRadio,
+} from "./SegmentedControl.styled";
const optionShape = PropTypes.shape({
name: PropTypes.node.isRequired,
@@ -37,26 +41,30 @@ export function SegmentedControl({
const isSelected = option.value === value;
const isFirst = index === 0;
const isLast = index === options.length - 1;
+ const id = `${name}-${option.value}`;
+ const labelId = `${name}-${option.value}`;
return (
- <SegmentedItem
- key={option.value}
- isSelected={isSelected}
- isFirst={isFirst}
- isLast={isLast}
- onClick={e => onChange(option.value)}
- selectedColor={option.selectedColor || "brand"}
- >
- {option.icon && <Icon name={option.icon} mr={1} />}
- <input
- id={`${name}-${option.value}`}
- className="Form-radio"
- type="radio"
- name={name}
- value={option.value}
- checked={isSelected}
- />
- <span>{option.name}</span>
- </SegmentedItem>
+ <li key={option.value}>
+ <SegmentedItem
+ id={labelId}
+ isSelected={isSelected}
+ isFirst={isFirst}
+ isLast={isLast}
+ selectedColor={option.selectedColor || "brand"}
+ >
+ {option.icon && <Icon name={option.icon} mr={1} />}
+ <SegmentedControlRadio
+ id={id}
+ name={name}
+ value={option.value}
+ checked={isSelected}
+ onChange={() => onChange(option.value)}
+ // Workaround for https://github.com/testing-library/dom-testing-library/issues/877
+ aria-labelledby={labelId}
+ />
+ {option.name}
+ </SegmentedItem>
+ </li>
);
})}
</SegmentedList>
diff --git a/frontend/src/metabase/components/SegmentedControl.styled.js b/frontend/src/metabase/components/SegmentedControl.styled.js
index 09e4651e126..0728869dcb0 100644
--- a/frontend/src/metabase/components/SegmentedControl.styled.js
+++ b/frontend/src/metabase/components/SegmentedControl.styled.js
@@ -7,7 +7,8 @@ export const SegmentedList = styled.ul`
display: flex;
`;
-export const SegmentedItem = styled.li`
+export const SegmentedItem = styled.label`
+ position: relative;
display: flex;
align-items: center;
font-weight: bold;
@@ -26,3 +27,16 @@ export const SegmentedItem = styled.li`
color: ${props => (!props.isSelected ? color(props.selectedColor) : null)};
}
`;
+
+export const SegmentedControlRadio = styled.input.attrs({ type: "radio" })`
+ cursor: inherit;
+ position: absolute;
+ opacity: 0;
+ width: 0;
+ height: 0;
+ top: 0;
+ left: 0;
+ margin: 0;
+ padding: 0;
+ z-index: 1;
+`;
diff --git a/frontend/src/metabase/css/components/form.css b/frontend/src/metabase/css/components/form.css
index 48318462c7d..53dc128379e 100644
--- a/frontend/src/metabase/css/components/form.css
+++ b/frontend/src/metabase/css/components/form.css
@@ -104,28 +104,6 @@
transition: border 300ms ease-in-out;
}
-.Form-radio {
- display: none;
-}
-
-.Form-radio + label {
- cursor: pointer;
- display: inline-block;
- flex: 0 0 auto;
- position: relative;
- margin-right: var(--margin-1);
- width: 8px;
- height: 8px;
- border: 2px solid white;
- box-shadow: 0 0 0 2px var(--color-shadow);
- border-radius: 8px;
-}
-
-.Form-radio:checked + label {
- box-shadow: 0 0 0 2px var(--color-shadow);
- background-color: var(--color-brand);
-}
-
.Form-field .AdminSelect {
border-color: var(--form-field-border-color);
}
diff --git a/frontend/src/metabase/visualizations/components/ChartSettings.jsx b/frontend/src/metabase/visualizations/components/ChartSettings.jsx
index 5881ffc5fa5..0a193f84bf1 100644
--- a/frontend/src/metabase/visualizations/components/ChartSettings.jsx
+++ b/frontend/src/metabase/visualizations/components/ChartSettings.jsx
@@ -247,6 +247,7 @@ class ChartSettings extends Component {
options={sectionNames}
optionNameFn={v => v}
optionValueFn={v => v}
+ optionKeyFn={v => v}
variant="bubble"
/>
);
diff --git a/frontend/test/metabase/visualizations/components/ChartSettings.unit.spec.js b/frontend/test/metabase/visualizations/components/ChartSettings.unit.spec.js
index c05aee5bd6a..5a00af3d289 100644
--- a/frontend/test/metabase/visualizations/components/ChartSettings.unit.spec.js
+++ b/frontend/test/metabase/visualizations/components/ChartSettings.unit.spec.js
@@ -20,10 +20,6 @@ function widget(widget = {}) {
};
}
-function expectTabSelected(node, value) {
- expect(node.parentNode.getAttribute("aria-selected")).toBe(String(value));
-}
-
describe("ChartSettings", () => {
it("should not crash if there are no widgets", () => {
render(<ChartSettings {...DEFAULT_PROPS} widgets={[]} />);
@@ -38,17 +34,17 @@ describe("ChartSettings", () => {
);
});
it("should default to the first section (if no section in DEFAULT_TAB_PRIORITY)", () => {
- const { getByText } = render(
+ const { getByLabelText } = render(
<ChartSettings
{...DEFAULT_PROPS}
widgets={[widget({ section: "Foo" }), widget({ section: "Bar" })]}
/>,
);
- expectTabSelected(getByText("Foo"), true);
- expectTabSelected(getByText("Bar"), false);
+ expect(getByLabelText("Foo")).toBeChecked();
+ expect(getByLabelText("Bar")).not.toBeChecked();
});
it("should default to the DEFAULT_TAB_PRIORITY", () => {
- const { getByText } = render(
+ const { getByLabelText } = render(
<ChartSettings
{...DEFAULT_PROPS}
widgets={[
@@ -57,21 +53,21 @@ describe("ChartSettings", () => {
]}
/>,
);
- expectTabSelected(getByText("Foo"), false);
- expectTabSelected(getByText("Display"), true);
+ expect(getByLabelText("Foo")).not.toBeChecked();
+ expect(getByLabelText("Display")).toBeChecked();
});
it("should be able to switch sections", () => {
- const { getByText } = render(
+ const { getByText, getByLabelText } = render(
<ChartSettings
{...DEFAULT_PROPS}
widgets={[widget({ section: "Foo" }), widget({ section: "Bar" })]}
/>,
);
- expectTabSelected(getByText("Foo"), true);
- expectTabSelected(getByText("Bar"), false);
+ expect(getByLabelText("Foo")).toBeChecked();
+ expect(getByLabelText("Bar")).not.toBeChecked();
fireEvent.click(getByText("Bar"));
- expectTabSelected(getByText("Foo"), false);
- expectTabSelected(getByText("Bar"), true);
+ expect(getByLabelText("Foo")).not.toBeChecked();
+ expect(getByLabelText("Bar")).toBeChecked();
});
it("should show widget names", () => {
--
GitLab