Remove redundant ParameterFieldWidget logic (#23271)

* Remove popover logic from ParameterFieldWidget

* Support formatting array values

* Fix isValid logic

* Readd close popover logic

* Fix various bugs/broken tests

Fix formatting/remapping

Fix text

Fix unit test

Fix text

Fix placeholder

Fix placeholder

Fix ellipses

Fix bug caused by the any type

Fix date parameter value formatting

Readd dashboard prop

Fix e2e test selector

frontend/src/metabase/components/DateAllOptionsWidget/DateAllOptionsWidget.tsx

@@ -7,7 +7,10 @@ import { dateParameterValueToMBQL } from "metabase/parameters/utils/mbql";
 import DatePicker from "metabase/query_builder/components/filters/pickers/DatePicker/DatePicker";
 import { filterToUrlEncoded } from "metabase/parameters/utils/date-formatting";
 
-import { Container, UpdateButton } from "./DateWidget.styled";
+import {
+  WidgetRoot,
+  UpdateButton,
+} from "metabase/parameters/components/widgets/Widget.styled";
 
 // Use a placeholder value as field references are not used in dashboard filters
 const noopRef = null;
@@ -39,7 +42,7 @@ const DateAllOptionsWidget = ({
     return filterValues.every((value: any) => value != null);
   };
   return (
-    <Container>
+    <WidgetRoot>
       <DatePicker
         filter={filter as any}
         onFilterChange={setFilter}
@@ -57,7 +60,7 @@ const DateAllOptionsWidget = ({
           {t`Update filter`}
         </UpdateButton>
       </DatePicker>
-    </Container>
+    </WidgetRoot>
   );
 };
 

frontend/src/metabase/components/FieldValuesWidget.jsx

@@ -356,10 +356,7 @@ function dedupeValues(valuesList) {
 }
 
 const LoadingState = () => (
-  <div
-    className="flex layout-centered align-center border-bottom"
-    style={{ minHeight: 82 }}
-  >
+  <div className="flex layout-centered align-center" style={{ minHeight: 82 }}>
     <LoadingSpinner size={32} />
   </div>
 );
@@ -386,7 +383,7 @@ const EveryOptionState = () => (
 );
 
 const OptionsMessage = ({ message }) => (
-  <div className="flex layout-centered p4 border-bottom">{message}</div>
+  <div className="flex layout-centered p4">{message}</div>
 );
 
 OptionsMessage.propTypes = optionsMessagePropTypes;

frontend/src/metabase/components/TextWidget/TextWidget.tsx

@@ -101,9 +101,7 @@ class TextWidget extends React.Component<Props, State> {
           changeFocus(false);
           this.setState({ value: this.props.value });
         }}
-        placeholder={
-          isEditing ? t`Enter a default value...` : defaultPlaceholder
-        }
+        placeholder={isEditing ? t`Enter a default value…` : defaultPlaceholder}
         disabled={disabled}
       />
     );

frontend/src/metabase/parameters/components/FormattedParameterValue/FormattedParameterValue.tsx

+import React from "react";
+
+import { formatParameterValue } from "metabase/parameters/utils/formatting";
+import { isDateParameter } from "metabase/parameters/utils/parameter-type";
+import { UiParameter, FieldFilterUiParameter } from "metabase/parameters/types";
+import ParameterFieldWidgetValue from "metabase/parameters/components/widgets/ParameterFieldWidget/ParameterFieldWidgetValue/ParameterFieldWidgetValue";
+
+type FormattedParameterValueProps = {
+  parameter: UiParameter;
+  value: unknown;
+  placeholder?: string;
+};
+
+function FormattedParameterValue({
+  parameter,
+  value,
+  placeholder,
+}: FormattedParameterValueProps) {
+  if (value == null) {
+    return placeholder;
+  }
+
+  if (hasFields(parameter) && !isDateParameter(parameter)) {
+    return (
+      <ParameterFieldWidgetValue fields={parameter.fields} value={value} />
+    );
+  }
+
+  return <span>{formatParameterValue(value, parameter)}</span>;
+}
+
+function hasFields(
+  parameter: UiParameter,
+): parameter is FieldFilterUiParameter {
+  return !!(parameter as FieldFilterUiParameter).fields;
+}
+
+export default FormattedParameterValue;

frontend/src/metabase/parameters/components/FormattedParameterValue/index.ts

+export { default } from "./FormattedParameterValue";

frontend/src/metabase/parameters/components/ParameterValueWidget.jsx

@@ -7,7 +7,7 @@ import _ from "underscore";
 import { getParameterIconName } from "metabase/parameters/utils/ui";
 import { isDashboardParameterWithoutMapping } from "metabase/parameters/utils/dashboards";
 import { isOnlyMappedToFields } from "metabase/parameters/utils/fields";
-import { formatParameterValue } from "metabase/parameters/utils/formatting";
+import { isDateParameter } from "metabase/parameters/utils/parameter-type";
 import PopoverWithTrigger from "metabase/components/PopoverWithTrigger";
 import Icon from "metabase/components/Icon";
 import DateSingleWidget from "metabase/components/DateSingleWidget";
@@ -19,6 +19,7 @@ import DateAllOptionsWidget from "metabase/components/DateAllOptionsWidget";
 import Tooltip from "metabase/components/Tooltip";
 import TextWidget from "metabase/components/TextWidget";
 import WidgetStatusIcon from "metabase/parameters/components/WidgetStatusIcon";
+import FormattedParameterValue from "metabase/parameters/components/FormattedParameterValue";
 
 import ParameterFieldWidget from "./widgets/ParameterFieldWidget/ParameterFieldWidget";
 import S from "./ParameterWidget.css";
@@ -139,7 +140,9 @@ class ParameterValueWidget extends Component {
       );
     } else {
       const placeholderText = isEditing
-        ? t`Select a default value…`
+        ? isDateParameter(parameter)
+          ? t`Select a default value…`
+          : t`Enter a default value…`
         : placeholder || t`Select…`;
 
       return (
@@ -165,9 +168,11 @@ class ParameterValueWidget extends Component {
                   />
                 )}
                 <div className="mr1 text-nowrap">
-                  {hasValue
-                    ? formatParameterValue(value, parameter)
-                    : placeholderText}
+                  <FormattedParameterValue
+                    parameter={parameter}
+                    value={value}
+                    placeholder={placeholderText}
+                  />
                 </div>
                 <WidgetStatusIcon
                   isFullscreen={isFullscreen}
@@ -240,7 +245,10 @@ function Widget({
         placeholder={placeholder}
         value={value}
         fields={parameter.fields}
-        setValue={setValue}
+        setValue={value => {
+          setValue(value);
+          onPopoverClose();
+        }}
         isEditing={isEditing}
         focusChanged={onFocusChanged}
       />

frontend/src/metabase/parameters/components/widgets/ParameterFieldWidget/ParameterFieldWidget.jsx

-import React, { Component } from "react";
-import ReactDOM from "react-dom";
+import React, { useState } from "react";
+import cx from "classnames";
 import PropTypes from "prop-types";
-
 import { t } from "ttag";
 import _ from "underscore";
 
 import FieldValuesWidget from "metabase/components/FieldValuesWidget";
-import ParameterFieldWidgetValue from "./ParameterFieldWidgetValue/ParameterFieldWidgetValue";
-import TippyPopover from "metabase/components/Popover/TippyPopover";
-import Button from "metabase/core/components/Button";
-
-import { normalizeValue } from "./normalizeValue";
-
 import { deriveFieldOperatorFromParameter } from "metabase/parameters/utils/operators";
-
-import cx from "classnames";
 import {
   getFilterArgumentFormatOptions,
   isEqualsOperator,
   isFuzzyOperator,
 } from "metabase/lib/schema_metadata";
+import {
+  WidgetRoot,
+  Footer,
+  UpdateButton,
+} from "metabase/parameters/components/widgets/Widget.styled";
+
+import { normalizeValue } from "./normalizeValue";
 
 const propTypes = {
-  dashboard: PropTypes.object,
   fields: PropTypes.array.isRequired,
   isEditing: PropTypes.bool.isRequired,
   parameter: PropTypes.object.isRequired,
   parameters: PropTypes.array.isRequired,
-  parentFocusChanged: PropTypes.bool,
   placeholder: PropTypes.string.isRequired,
   setValue: PropTypes.func.isRequired,
   value: PropTypes.string,
-  target: PropTypes.instanceOf(Element).isRequired,
+  dashboard: PropTypes.object,
 };
 
-const BORDER_WIDTH = 1;
-
-export default class ParameterFieldWidget extends Component {
-  constructor(props) {
-    super(props);
-    this.state = {
-      isFocused: false,
-      value: props.value,
-      widgetWidth: null,
-    };
-  }
-
-  static noPopover = true;
-
-  UNSAFE_componentWillReceiveProps(nextProps) {
-    if (this.props.value !== nextProps.value) {
-      this.setState({ value: nextProps.value });
-    }
-  }
-
-  componentDidUpdate() {
-    const element = ReactDOM.findDOMNode(this._unfocusedElement);
-    if (!this.state.isFocused && element) {
-      const parameterWidgetElement = element.parentNode.parentNode.parentNode;
-      if (parameterWidgetElement.clientWidth !== this.state.widgetWidth) {
-        this.setState({ widgetWidth: parameterWidgetElement.clientWidth });
-      }
-    }
-  }
-
-  render() {
-    const {
-      setValue,
-      isEditing,
-      fields,
-      parentFocusChanged,
-      parameter,
-      parameters,
-      dashboard,
-    } = this.props;
-    const operator = deriveFieldOperatorFromParameter(parameter);
-    const { isFocused, widgetWidth } = this.state;
-    const { numFields = 1, multi = false, verboseName } = operator || {};
-    const savedValue = normalizeValue(this.props.value);
-    const unsavedValue = normalizeValue(this.state.value);
-    const isEqualsOp = isEqualsOperator(operator);
-    const disableSearch = operator && isFuzzyOperator(operator);
-    const defaultPlaceholder = isFocused
-      ? ""
-      : this.props.placeholder || t`Enter a value...`;
-
-    const focusChanged = isFocused => {
-      if (parentFocusChanged) {
-        parentFocusChanged(isFocused);
-      }
-      this.setState({ isFocused });
-    };
-
-    const footerClassName = cx(
-      "flex mt1 px1 pb1 PopoverFooter PopoverParameterFieldWidgetFooter",
-      isEqualsOp ? "mr1 mb1" : "PopoverFooterWhenIsNotEqualOps",
-    );
-
-    const placeholder = isEditing
-      ? t`Enter a default value...`
-      : defaultPlaceholder;
-
-    if (!isFocused) {
-      return (
-        <div
-          ref={_ => (this._unfocusedElement = _)}
-          className="flex-full cursor-pointer"
-          onClick={() => focusChanged(true)}
-        >
-          {savedValue.length > 0 ? (
-            <ParameterFieldWidgetValue
-              savedValue={savedValue}
+export default function ParameterFieldWidget({
+  value,
+  setValue,
+  isEditing,
+  fields,
+  parameter,
+  parameters,
+  placeholder = t`Enter a value...`,
+  dashboard,
+}) {
+  const [unsavedValue, setUnsavedValue] = useState(() => normalizeValue(value));
+  const operator = deriveFieldOperatorFromParameter(parameter);
+  const { numFields = 1, multi = false, verboseName } = operator || {};
+  const isEqualsOp = isEqualsOperator(operator);
+  const disableSearch = operator && isFuzzyOperator(operator);
+  const hasValue = Array.isArray(value) ? value.length > 0 : value != null;
+
+  const isValid =
+    unsavedValue.every(value => value != null) &&
+    (multi || unsavedValue.length === numFields);
+
+  return (
+    <WidgetRoot>
+      <div className="p1">
+        {verboseName && !isEqualsOp && (
+          <div className="text-bold mb1">{verboseName}...</div>
+        )}
+
+        {_.times(numFields, index => {
+          const value = multi ? unsavedValue : [unsavedValue[index]];
+          const onValueChange = multi
+            ? newValues => setUnsavedValue(newValues)
+            : ([value]) => {
+                const newValues = [...unsavedValue];
+                newValues[index] = value;
+                setUnsavedValue(newValues);
+              };
+          return (
+            <FieldValuesWidget
+              key={index}
+              className={cx("input", numFields - 1 !== index && "mb1")}
+              value={value}
+              parameter={parameter}
+              parameters={parameters}
+              dashboard={dashboard}
+              onChange={onValueChange}
+              placeholder={isEditing ? t`Enter a default value…` : undefined}
               fields={fields}
+              autoFocus={index === 0}
+              multi={multi}
+              disableSearch={disableSearch}
+              formatOptions={
+                operator && getFilterArgumentFormatOptions(operator, index)
+              }
+              color="brand"
+              minWidth={300}
+              maxWidth={400}
             />
-          ) : (
-            <span>{placeholder}</span>
-          )}
-        </div>
-      );
-    } else {
-      return (
-        <TippyPopover
-          reference={this.props.target}
-          placement="bottom-start"
-          visible
-          onClose={() => focusChanged(false)}
-          content={
-            <div
-              className={cx("relative PopoverBody--marginBottom", {
-                p2: !isEqualsOp,
-              })}
-            >
-              {verboseName && !isEqualsOp && (
-                <div className="text-bold mb1">{verboseName}...</div>
-              )}
-
-              {_.times(numFields, index => {
-                const value = multi ? unsavedValue : [unsavedValue[index]];
-                const onValueChange = multi
-                  ? newValues => this.setState({ value: newValues })
-                  : ([value]) => {
-                      const newValues = [...unsavedValue];
-                      newValues[index] = value;
-                      this.setState({ value: newValues });
-                    };
-                return (
-                  <FieldValuesWidget
-                    key={index}
-                    className={cx("input", numFields - 1 !== index && "mb1")}
-                    value={value}
-                    parameter={parameter}
-                    parameters={parameters}
-                    dashboard={dashboard}
-                    onChange={onValueChange}
-                    placeholder={placeholder}
-                    fields={fields}
-                    autoFocus={index === 0}
-                    multi={multi}
-                    disableSearch={disableSearch}
-                    formatOptions={
-                      operator &&
-                      getFilterArgumentFormatOptions(operator, index)
-                    }
-                    color="brand"
-                    style={{
-                      borderWidth: BORDER_WIDTH,
-                      minWidth: widgetWidth
-                        ? widgetWidth + BORDER_WIDTH * 2
-                        : null,
-                    }}
-                    minWidth={300}
-                    maxWidth={400}
-                  />
-                );
-              })}
-              <div className={footerClassName}>
-                <Button
-                  primary
-                  className="ml-auto"
-                  disabled={
-                    savedValue.length === 0 && unsavedValue.length === 0
-                  }
-                  onClick={() => {
-                    setValue(unsavedValue.length > 0 ? unsavedValue : null);
-                    focusChanged(false);
-                  }}
-                >
-                  {savedValue.length > 0 ? t`Update filter` : t`Add filter`}
-                </Button>
-              </div>
-            </div>
-          }
-        />
-      );
-    }
-  }
+          );
+        })}
+      </div>
+      <Footer>
+        <UpdateButton
+          disabled={!isValid}
+          onClick={() => {
+            setValue(unsavedValue);
+          }}
+        >
+          {hasValue ? t`Update filter` : t`Add filter`}
+        </UpdateButton>
+      </Footer>
+    </WidgetRoot>
+  );
 }
 
 ParameterFieldWidget.propTypes = propTypes;

frontend/src/metabase/parameters/components/widgets/ParameterFieldWidget/ParameterFieldWidgetValue/ParameterFieldWidgetValue.jsx → frontend/src/metabase/parameters/components/widgets/ParameterFieldWidget/ParameterFieldWidgetValue/ParameterFieldWidgetValue.tsx

 import React from "react";
 import PropTypes from "prop-types";
-import { ngettext, msgid } from "ttag";
 
+import { renderNumberOfSelections } from "metabase/parameters/utils/formatting";
 import Value from "metabase/components/Value";
+import Field from "metabase-lib/lib/metadata/Field";
+
 import { normalizeValue } from "../normalizeValue";
 
-function renderNumberOfSelections(numberOfSelections) {
-  return ngettext(
-    msgid`${numberOfSelections} selection`,
-    `${numberOfSelections} selections`,
-    numberOfSelections,
-  );
-}
+type ParameterFieldWidgetValueProps = {
+  value: unknown;
+  fields: Field[];
+};
 
-export default function ParameterFieldWidgetValue({ savedValue, fields }) {
-  const values = normalizeValue(savedValue);
+export default function ParameterFieldWidgetValue({
+  value,
+  fields,
+}: ParameterFieldWidgetValueProps) {
+  const values = normalizeValue(value);
 
   const numberOfValues = values.length;
 
@@ -23,13 +25,13 @@ export default function ParameterFieldWidgetValue({ savedValue, fields }) {
   const shouldRemap = fields.length === 1;
 
   return numberOfValues > 1 ? (
-    renderNumberOfSelections(numberOfValues)
+    <>{renderNumberOfSelections(numberOfValues)}</>
   ) : (
-    <Value remap={shouldRemap} value={savedValue[0]} column={fields[0]} />
+    <Value remap={shouldRemap} value={values[0]} column={fields[0]} />
   );
 }
 
 ParameterFieldWidgetValue.propTypes = {
-  savedValue: PropTypes.array,
+  value: PropTypes.array,
   fields: PropTypes.array,
 };

frontend/src/metabase/parameters/components/widgets/ParameterFieldWidget/ParameterFieldWidgetValue/ParameterFieldWidgetValue.unit.spec.js

@@ -6,15 +6,13 @@ import { render, screen } from "@testing-library/react";
 const value = "A value";
 
 describe("when fields is empty array", () => {
-  it("renders savedValue if it is a single item", () => {
-    render(<ParameterFieldWidgetValue savedValue={[value]} fields={[]} />);
+  it("renders value if it is a single item", () => {
+    render(<ParameterFieldWidgetValue value={[value]} fields={[]} />);
     screen.getByText(value);
   });
 
   it("renders number of selections if multiple items", () => {
-    render(
-      <ParameterFieldWidgetValue savedValue={[value, value]} fields={[]} />,
-    );
+    render(<ParameterFieldWidgetValue value={[value, value]} fields={[]} />);
     screen.getByText("2 selections");
   });
 });

frontend/src/metabase/parameters/components/widgets/ParameterFieldWidget/normalizeValue.js → frontend/src/metabase/parameters/components/widgets/ParameterFieldWidget/normalizeValue.ts

-export function normalizeValue(value) {
+export function normalizeValue(value: unknown) {
   if (Array.isArray(value)) {
     return value;
   }

frontend/src/metabase/components/DateAllOptionsWidget/DateWidget.styled.tsx → frontend/src/metabase/parameters/components/widgets/Widget.styled.tsx

@@ -4,7 +4,7 @@ import { color } from "metabase/lib/colors";
 import { space } from "metabase/styled-components/theme";
 import Button from "metabase/core/components/Button";
 
-export const Container = styled.div`
+export const WidgetRoot = styled.div`
   min-width: 300px;
 `;
 

frontend/src/metabase/parameters/utils/formatting.ts

+import { ngettext, msgid } from "ttag";
+
 import { formatValue } from "metabase/lib/formatting";
 
 import { getParameterType, isFieldFilterParameter } from "./parameter-type";
@@ -24,10 +26,16 @@ function formatWithInferredType(value: any, parameter: UiParameter) {
   });
 }
 
-export function formatParameterValue(value: any, parameter: UiParameter) {
+export function formatParameterValue(value: unknown, parameter: UiParameter) {
+  if (Array.isArray(value) && value.length > 1) {
+    return renderNumberOfSelections(value.length);
+  }
+
+  value = Array.isArray(value) ? value[0] : value;
+
   const type = getParameterType(parameter);
   if (type === "date") {
-    return formatDateValue(value, parameter);
+    return formatDateValue(String(value), parameter);
   }
 
   if (isFieldFilterParameter(parameter)) {
@@ -54,3 +62,11 @@ export function formatParameterValue(value: any, parameter: UiParameter) {
   // infer type information from parameter type
   return formatWithInferredType(value, parameter);
 }
+
+export function renderNumberOfSelections(numberOfSelections: number) {
+  return ngettext(
+    msgid`${numberOfSelections} selection`,
+    `${numberOfSelections} selections`,
+    numberOfSelections,
+  );
+}

frontend/src/metabase/parameters/utils/formatting.unit.spec.ts

@@ -84,6 +84,18 @@ describe("metabase/parameters/utils/formatting", () => {
         expected: "foo",
         fields: [],
       },
+      {
+        type: "number/=",
+        value: [1, 2, 3, 4, 5],
+        expected: "5 selections",
+        fields: [numberField],
+      },
+      {
+        type: "number/=",
+        value: [1],
+        expected: "1",
+        fields: [numberField],
+      },
     ];
 
     test.each(cases)(

frontend/test/metabase/scenarios/admin/tools/erroring-questions.cy.spec.js

@@ -113,7 +113,7 @@ function fixQuestion(name) {
   cy.findByText("Open Editor").click();
 
   cy.icon("variable").click();
-  cy.findByPlaceholderText("Enter a default value...").type("Foo");
+  cy.findByPlaceholderText("Enter a default value…").type("Foo");
 
   cy.findByText("Save").click();
 

frontend/test/metabase/scenarios/embedding/embedding-native.cy.spec.js

@@ -103,8 +103,10 @@ describe("scenarios > embedding > native questions", () => {
 
       // Let's try to remove one filter
       cy.findByText("Q2, 2018")
-        .siblings(".Icon-close")
-        .click();
+        .closest("fieldset")
+        .within(() => {
+          cy.icon("close").click();
+        });
 
       // Order ID is 926 - there should be only one result after this
       filterWidget()

frontend/test/metabase/scenarios/native-filters/helpers/e2e-field-filter-helpers.js

@@ -121,7 +121,7 @@ export function mapTo({ table, field } = {}) {
  */
 export function openEntryForm(isFilterRequired) {
   const selector = isFilterRequired
-    ? cy.findByText("Enter a default value...")
+    ? cy.findByText("Enter a default value…")
     : filterWidget();
 
   selector.click();
@@ -161,7 +161,7 @@ function addSimpleNumberFilter(value) {
  * @param {string} value
  */
 function enterDefaultValue(value) {
-  cy.findByPlaceholderText("Enter a default value...").type(`${value}{enter}`);
+  cy.findByPlaceholderText("Enter a default value…").type(`${value}{enter}`);
   cy.button("Add filter").click();
 }
 
@@ -170,7 +170,7 @@ function enterDefaultValue(value) {
  * @param {string} result
  */
 export function pickDefaultValue(searchTerm, result) {
-  cy.findByPlaceholderText("Enter a default value...").type(searchTerm);
+  cy.findByPlaceholderText("Enter a default value…").type(searchTerm);
   popover()
     .findByText(result)
     .click();

frontend/test/metabase/scenarios/native-filters/helpers/e2e-sql-filter-helpers.js

@@ -57,7 +57,7 @@ export function setWidgetValue(value) {
  * @param {string} value
  */
 export function setDefaultValue(value) {
-  cy.findByPlaceholderText("Enter a default value...").type(value);
+  cy.findByPlaceholderText("Enter a default value…").type(value);
 }
 
 // UI PATTERNS