diff --git a/frontend/src/metabase/actions/containers/ActionCreator/FormCreator/FieldSettingsPopover.styled.tsx b/frontend/src/metabase/actions/containers/ActionCreator/FormCreator/FieldSettingsPopover.styled.tsx
index 0420f21a19993cfa8ac7e8e76b0b387c3495da7a..edca3060dfe2e5a462ec2e3e8e581c4be86abc5f 100644
--- a/frontend/src/metabase/actions/containers/ActionCreator/FormCreator/FieldSettingsPopover.styled.tsx
+++ b/frontend/src/metabase/actions/containers/ActionCreator/FormCreator/FieldSettingsPopover.styled.tsx
@@ -8,13 +8,18 @@ export const SettingsPopoverBody = styled.div`
padding: ${space(3)};
`;
-export const SectionLabel = styled.div`
+export const SectionLabel = styled.label`
+ display: block;
color: ${color("text-medium")};
font-weight: bold;
padding-left: ${space(0)};
margin-bottom: ${space(1)};
`;
+export const RequiredToggleLabel = styled.label`
+ font-weight: bold;
+`;
+
export const Divider = styled.div`
border-bottom: 1px solid ${color("border")};
margin: ${space(2)} 0;
diff --git a/frontend/src/metabase/actions/containers/ActionCreator/FormCreator/FieldSettingsPopover.tsx b/frontend/src/metabase/actions/containers/ActionCreator/FormCreator/FieldSettingsPopover.tsx
index 2828b08380314664c882b220f3a79615a8951eca..e80752291b0450c6ca9108d0eaf33b1982b81644 100644
--- a/frontend/src/metabase/actions/containers/ActionCreator/FormCreator/FieldSettingsPopover.tsx
+++ b/frontend/src/metabase/actions/containers/ActionCreator/FormCreator/FieldSettingsPopover.tsx
@@ -12,22 +12,27 @@ import Radio from "metabase/core/components/Radio";
import Toggle from "metabase/core/components/Toggle";
import TippyPopoverWithTrigger from "metabase/components/PopoverWithTrigger/TippyPopoverWithTrigger";
+import { isEmpty } from "metabase/lib/validate";
+
import { getFieldTypes, getInputTypes } from "./constants";
import {
SettingsTriggerIcon,
ToggleContainer,
SettingsPopoverBody,
SectionLabel,
+ RequiredToggleLabel,
Divider,
} from "./FieldSettingsPopover.styled";
+export interface FieldSettingsPopoverProps {
+ fieldSettings: FieldSettings;
+ onChange: (fieldSettings: FieldSettings) => void;
+}
+
export function FieldSettingsPopover({
fieldSettings,
onChange,
-}: {
- fieldSettings: FieldSettings;
- onChange: (fieldSettings: FieldSettings) => void;
-}) {
+}: FieldSettingsPopoverProps) {
return (
<TippyPopoverWithTrigger
placement="bottom-end"
@@ -50,6 +55,19 @@ export function FieldSettingsPopover({
);
}
+function cleanDefaultValue(fieldType: FieldType, value?: string | number) {
+ if (isEmpty(value)) {
+ return;
+ }
+
+ if (fieldType === "number") {
+ const clean = Number(value);
+ return !Number.isNaN(clean) ? clean : 0;
+ }
+
+ return value;
+}
+
export function FormCreatorPopoverBody({
fieldSettings,
onChange,
@@ -88,10 +106,7 @@ export function FormCreatorPopoverBody({
onChange({
...fieldSettings,
required,
- defaultValue:
- fieldSettings.fieldType === "number"
- ? Number(defaultValue)
- : defaultValue,
+ defaultValue: cleanDefaultValue(fieldSettings.fieldType, defaultValue),
});
const hasPlaceholder =
@@ -208,16 +223,18 @@ function RequiredInput({
return (
<div>
<ToggleContainer>
- <strong>{t`Required`}</strong>
+ <RequiredToggleLabel htmlFor="is-required">{t`Required`}</RequiredToggleLabel>
<Toggle
+ id="is-required"
value={!!value}
onChange={required => onChange({ required, defaultValue })}
/>
</ToggleContainer>
{!value && (
<>
- <SectionLabel>{t`Default Value`}</SectionLabel>
+ <SectionLabel htmlFor="default-value">{t`Default value`}</SectionLabel>
<Input
+ id="default-value"
fullWidth
value={defaultValue ?? ""}
onChange={e =>
diff --git a/frontend/src/metabase/actions/containers/ActionCreator/FormCreator/FieldSettingsPopover.unit.spec.tsx b/frontend/src/metabase/actions/containers/ActionCreator/FormCreator/FieldSettingsPopover.unit.spec.tsx
index 4e1b47629a6b3089ad31d410c99f31e3c1defe42..408a39ce17254c475e82c964deaff4879cc1a160 100644
--- a/frontend/src/metabase/actions/containers/ActionCreator/FormCreator/FieldSettingsPopover.unit.spec.tsx
+++ b/frontend/src/metabase/actions/containers/ActionCreator/FormCreator/FieldSettingsPopover.unit.spec.tsx
@@ -1,20 +1,45 @@
-import React from "react";
+import React, { useState } from "react";
import { render, screen } from "@testing-library/react";
import userEvent from "@testing-library/user-event";
-
+import type { FieldSettings } from "metabase-types/api";
import { getDefaultFieldSettings } from "../../../utils";
-import { FieldSettingsPopover } from "./FieldSettingsPopover";
+import {
+ FieldSettingsPopover,
+ FieldSettingsPopoverProps,
+} from "./FieldSettingsPopover";
+
+function WrappedFieldSettingsPopover({
+ fieldSettings: initialSettings,
+ onChange,
+}: FieldSettingsPopoverProps) {
+ const [settings, setSettings] = useState(initialSettings);
+
+ const handleChange = (nextSettings: FieldSettings) => {
+ setSettings(nextSettings);
+ onChange(nextSettings);
+ };
+
+ return (
+ <FieldSettingsPopover fieldSettings={settings} onChange={handleChange} />
+ );
+}
+
+function setup({ settings = getDefaultFieldSettings() } = {}) {
+ const onChange = jest.fn();
+ render(
+ <WrappedFieldSettingsPopover
+ fieldSettings={settings}
+ onChange={onChange}
+ />,
+ );
+ return { settings, onChange };
+}
describe("actions > FormCreator > FieldSettingsPopover", () => {
it("should show the popover", async () => {
- const changeSpy = jest.fn();
- const settings = getDefaultFieldSettings();
-
- render(
- <FieldSettingsPopover fieldSettings={settings} onChange={changeSpy} />,
- );
+ setup();
- await userEvent.click(screen.getByLabelText("Field settings"));
+ userEvent.click(screen.getByLabelText("Field settings"));
expect(
await screen.findByTestId("field-settings-popover"),
@@ -22,24 +47,18 @@ describe("actions > FormCreator > FieldSettingsPopover", () => {
});
it("should fire onChange handler clicking a different field type", async () => {
- const changeSpy = jest.fn();
- const settings = getDefaultFieldSettings();
-
- render(
- <FieldSettingsPopover fieldSettings={settings} onChange={changeSpy} />,
- );
+ const { settings, onChange } = setup();
- await userEvent.click(screen.getByLabelText("Field settings"));
+ userEvent.click(screen.getByLabelText("Field settings"));
expect(
await screen.findByTestId("field-settings-popover"),
).toBeInTheDocument();
- await userEvent.click(screen.getByText("Date"));
+ userEvent.click(screen.getByText("Date"));
- expect(changeSpy).toHaveBeenCalledTimes(1);
-
- expect(changeSpy).toHaveBeenCalledWith({
+ expect(onChange).toHaveBeenCalledTimes(1);
+ expect(onChange).toHaveBeenCalledWith({
...settings,
fieldType: "date",
inputType: "date", // should set default input type for new field type
@@ -47,38 +66,27 @@ describe("actions > FormCreator > FieldSettingsPopover", () => {
});
it("should fire onChange handler clicking a different input type", async () => {
- const changeSpy = jest.fn();
- const settings = getDefaultFieldSettings();
-
- render(
- <FieldSettingsPopover fieldSettings={settings} onChange={changeSpy} />,
- );
+ const { settings, onChange } = setup();
- await userEvent.click(screen.getByLabelText("Field settings"));
+ userEvent.click(screen.getByLabelText("Field settings"));
expect(
await screen.findByTestId("field-settings-popover"),
).toBeInTheDocument();
- await userEvent.click(screen.getByText("Dropdown"));
+ userEvent.click(screen.getByText("Dropdown"));
- expect(changeSpy).toHaveBeenCalledTimes(1);
-
- expect(changeSpy).toHaveBeenCalledWith({
+ expect(onChange).toHaveBeenCalledTimes(1);
+ expect(onChange).toHaveBeenCalledWith({
...settings,
inputType: "select",
});
});
it("should fire onChange handler editing placeholder", async () => {
- const changeSpy = jest.fn();
- const settings = getDefaultFieldSettings();
-
- render(
- <FieldSettingsPopover fieldSettings={settings} onChange={changeSpy} />,
- );
+ const { settings, onChange } = setup();
- await userEvent.click(screen.getByLabelText("Field settings"));
+ userEvent.click(screen.getByLabelText("Field settings"));
expect(
await screen.findByTestId("field-settings-popover"),
@@ -86,9 +94,38 @@ describe("actions > FormCreator > FieldSettingsPopover", () => {
await userEvent.type(screen.getByTestId("placeholder-input"), "$");
- expect(changeSpy).toHaveBeenLastCalledWith({
+ expect(onChange).toHaveBeenLastCalledWith({
...settings,
placeholder: "$",
});
});
+
+ it("should fire onChange handler after changing required and default value properties", async () => {
+ const settings = getDefaultFieldSettings({
+ fieldType: "number",
+ required: true,
+ });
+ const { onChange } = setup({ settings });
+
+ userEvent.click(screen.getByLabelText("Field settings"));
+ await screen.findByTestId("field-settings-popover");
+
+ userEvent.click(screen.getByLabelText("Required"));
+ expect(onChange).toHaveBeenCalledTimes(1);
+ expect(onChange).toHaveBeenLastCalledWith({
+ ...settings,
+ defaultValue: undefined,
+ required: false,
+ });
+
+ const defaultValueInput = screen.getByLabelText("Default value");
+ expect(defaultValueInput).not.toHaveValue();
+ await userEvent.type(defaultValueInput, "5");
+
+ expect(onChange).toHaveBeenLastCalledWith({
+ ...settings,
+ required: false,
+ defaultValue: 5,
+ });
+ });
});