Redundant separator displayed in table filter popup (#32172)

frontend/src/metabase/components/Popover/Popover.css

@@ -177,7 +177,7 @@
   border-bottom-right-radius: 6px;
   border-bottom-left-radius: 6px;
   padding-top: 8px;
-  width: calc(100% - 2px);
+  width: 100%;
   /* Without z-index; calendar days, if selected, scroll above this component */
   z-index: 1;
 }

frontend/src/metabase/query_builder/components/filters/FilterPopover/FilterPopover.styled.tsx

@@ -20,3 +20,16 @@ export const Button = styled(BaseButton)<Props>`
       alpha(primaryColor, 0.8)};
   }
 `;
+
+export const FilterPopoverSeparator = styled.hr`
+  border: 0;
+  height: 0;
+  border-top: 1px solid ${color("border")};
+`;
+
+// Mimics the .PopoverBody--marginBottom class in Popover.css that the other
+// filter pickers use to keep the PopoverFooter from overlapping with the
+// content of the picker.
+export const EmptyFilterPickerPlaceholder = styled.div`
+  margin-bottom: 60px;
+`;

frontend/src/metabase/query_builder/components/filters/FilterPopover/FilterPopover.tsx

@@ -21,7 +21,11 @@ import DatePicker from "../pickers/DatePicker/DatePicker";
 import TimePicker from "../pickers/TimePicker";
 import { DateShortcutOptions } from "../pickers/DatePicker/DatePickerShortcutOptions";
 import DimensionList from "../../DimensionList";
-import { Button } from "./FilterPopover.styled";
+import {
+  Button,
+  EmptyFilterPickerPlaceholder,
+  FilterPopoverSeparator,
+} from "./FilterPopover.styled";
 import FilterPopoverFooter from "./FilterPopoverFooter";
 import FilterPopoverPicker from "./FilterPopoverPicker";
 import FilterPopoverHeader from "./FilterPopoverHeader";
@@ -238,6 +242,9 @@ export default function FilterPopover({
   const shouldShowDatePicker = field?.isDate() && !field?.isTime();
   const supportsExpressions = query.database()?.supportsExpressions();
 
+  const filterOperator = filter.operator();
+  const hasPicker = filterOperator && filterOperator.fields.length > 0;
+
   return (
     <div className={className} style={{ minWidth: MIN_WIDTH, ...style }}>
       {shouldShowDatePicker ? (
@@ -286,15 +293,22 @@ export default function FilterPopover({
                 showFieldPicker={showFieldPicker}
                 forceShowOperatorSelector={showOperatorSelector}
               />
-              <FilterPopoverPicker
-                className="px1 pt1 pb1"
-                filter={filter}
-                onFilterChange={handleFilterChange}
-                onCommit={handleCommit}
-                maxWidth={MAX_WIDTH}
-                primaryColor={primaryColor}
-                checkedColor={checkedColor}
-              />
+              {hasPicker ? (
+                <>
+                  <FilterPopoverSeparator data-testid="filter-popover-separator" />
+                  <FilterPopoverPicker
+                    className="px1 pt1 pb1"
+                    filter={filter}
+                    onFilterChange={handleFilterChange}
+                    onCommit={handleCommit}
+                    maxWidth={MAX_WIDTH}
+                    primaryColor={primaryColor}
+                    checkedColor={checkedColor}
+                  />
+                </>
+              ) : (
+                <EmptyFilterPickerPlaceholder data-testid="empty-picker-placeholder" />
+              )}
             </>
           )}
           <FilterPopoverFooter

frontend/src/metabase/query_builder/components/filters/FilterPopover/FilterPopover.unit.spec.tsx

 import "__support__/ui-mocks";
 
-import { render, screen } from "@testing-library/react";
+import { screen } from "@testing-library/react";
 import userEvent from "@testing-library/user-event";
 
 import { createMockMetadata } from "__support__/metadata";
@@ -14,6 +14,7 @@ import {
 } from "metabase-types/api/mocks/presets";
 import Question from "metabase-lib/Question";
 import Filter from "metabase-lib/queries/structured/Filter";
+import StructuredQuery from "metabase-lib/queries/StructuredQuery";
 import FilterPopover from "./FilterPopover";
 
 const metadata = createMockMetadata({
@@ -36,67 +37,59 @@ const QUERY = Question.create({
     ["field", PRODUCTS.TITLE, { "source-field": ORDERS.PRODUCT_ID }],
     "asdf",
   ]);
-
-const [RELATIVE_DAY_FILTER, NUMERIC_FILTER, STRING_CONTAINS_FILTER] =
+const [RELATIVE_DAY_FILTER, NUMERIC_FILTER, STRING_CONTAINS_FILTER]: Filter[] =
   QUERY.filters();
 
 const dummyFunction = jest.fn();
 
+const setup = ({
+  filter,
+  onChange = dummyFunction,
+  onChangeFilter = dummyFunction,
+  showFieldPicker = true,
+}: {
+  filter: Filter;
+  query?: StructuredQuery;
+  onChange?: (filter: Filter) => void;
+  onChangeFilter?: (filter: Filter) => void;
+  showFieldPicker?: boolean;
+}) => {
+  renderWithProviders(
+    <FilterPopover
+      query={QUERY}
+      filter={filter}
+      onChange={onChange}
+      onChangeFilter={onChangeFilter}
+      showFieldPicker={showFieldPicker}
+    />,
+  );
+};
+
 describe("FilterPopover", () => {
   describe("existing filter", () => {
     describe("DatePicker", () => {
       it("should render a date picker for a date filter", () => {
-        const filter = new Filter(RELATIVE_DAY_FILTER, null, QUERY);
-
-        render(
-          <FilterPopover
-            query={QUERY}
-            filter={filter}
-            onChangeFilter={dummyFunction}
-          />,
-        );
+        setup({ filter: RELATIVE_DAY_FILTER });
 
         expect(screen.getByTestId("date-picker")).toBeInTheDocument();
       });
     });
     describe("filter operator selection", () => {
       it("should have an operator selector", () => {
-        const filter = new Filter(NUMERIC_FILTER, null, QUERY);
-        renderWithProviders(
-          <FilterPopover
-            query={QUERY}
-            filter={filter}
-            onChangeFilter={dummyFunction}
-          />,
-        );
+        setup({ filter: NUMERIC_FILTER });
         expect(screen.getByText("Equal to")).toBeInTheDocument();
         expect(screen.getByText("1,234")).toBeInTheDocument();
       });
     });
     describe("filter options", () => {
       it("should not show a control to the user if the filter has no options", () => {
-        const filter = new Filter(RELATIVE_DAY_FILTER, null, QUERY);
-        renderWithProviders(
-          <FilterPopover
-            query={QUERY}
-            filter={filter}
-            onChange={dummyFunction}
-            onChangeFilter={dummyFunction}
-          />,
-        );
+        setup({ filter: RELATIVE_DAY_FILTER });
         expect(screen.queryByText("Include")).not.toBeInTheDocument();
         expect(screen.queryByText("today")).not.toBeInTheDocument();
       });
 
       it('should show "case-sensitive" option to the user for "contains" filters', () => {
-        const filter = new Filter(STRING_CONTAINS_FILTER, null, QUERY);
-        renderWithProviders(
-          <FilterPopover
-            query={QUERY}
-            filter={filter}
-            onChangeFilter={dummyFunction}
-          />,
-        );
+        setup({ filter: STRING_CONTAINS_FILTER });
         expect(screen.getByText("Case sensitive")).toBeInTheDocument();
       });
 
@@ -104,14 +97,7 @@ describe("FilterPopover", () => {
       // Tried to click on checkbox, label, their parent - nothing seems to be working, while it works fine in UI
       // eslint-disable-next-line jest/no-disabled-tests, jest/expect-expect
       it.skip("should let the user toggle an option", async () => {
-        const filter = new Filter(RELATIVE_DAY_FILTER, null, QUERY);
-        renderWithProviders(
-          <FilterPopover
-            query={QUERY}
-            filter={filter}
-            onChangeFilter={dummyFunction}
-          />,
-        );
+        setup({ filter: RELATIVE_DAY_FILTER });
         const ellipsis = screen.getByLabelText("ellipsis icon");
         userEvent.click(ellipsis);
         const includeToday = await screen.findByText("Include today");
@@ -120,14 +106,7 @@ describe("FilterPopover", () => {
 
       // eslint-disable-next-line jest/no-disabled-tests
       it.skip("should let the user toggle a date filter type", async () => {
-        const filter = new Filter(RELATIVE_DAY_FILTER, null, QUERY);
-        renderWithProviders(
-          <FilterPopover
-            query={QUERY}
-            filter={filter}
-            onChangeFilter={dummyFunction}
-          />,
-        );
+        setup({ filter: RELATIVE_DAY_FILTER });
         const back = screen.getByLabelText("chevronleft icon");
         userEvent.click(back);
         expect(
@@ -137,14 +116,7 @@ describe("FilterPopover", () => {
 
       // eslint-disable-next-line jest/no-disabled-tests
       it.skip("should let the user toggle a text filter type", async () => {
-        const filter = new Filter(STRING_CONTAINS_FILTER, null, QUERY);
-        renderWithProviders(
-          <FilterPopover
-            query={QUERY}
-            filter={filter}
-            onChangeFilter={dummyFunction}
-          />,
-        );
+        setup({ filter: STRING_CONTAINS_FILTER });
         userEvent.click(await screen.findByText("Contains"));
         userEvent.click(await screen.findByText("Is"));
 
@@ -154,4 +126,45 @@ describe("FilterPopover", () => {
       });
     });
   });
+  describe("filter rendering", () => {
+    describe("no-value filters", () => {
+      it.each(["is-null", "not-null", "is-empty", "not-empty"])(
+        "should not render picker or separator when selecting '%s' filter from the column dropdown",
+        async operator => {
+          setup({
+            filter: new Filter(
+              [operator, ["field", PRODUCTS.TITLE, null], null],
+              null,
+              QUERY,
+            ),
+            showFieldPicker: false,
+          });
+
+          expect(
+            screen.getByTestId("empty-picker-placeholder"),
+          ).toBeInTheDocument();
+        },
+      );
+    });
+
+    describe("non datetime filters", () => {
+      it.each([
+        { filter: STRING_CONTAINS_FILTER, label: "contains" },
+        { filter: NUMERIC_FILTER, label: "equals" },
+      ])(
+        "should render the default filter picker and separator if the $label filter has arguments",
+        async ({ filter }) => {
+          setup({ filter });
+
+          expect(
+            screen.getByTestId("filter-popover-separator"),
+          ).toBeInTheDocument();
+
+          expect(
+            screen.queryByTestId("default-filter-picker"),
+          ).not.toBeInTheDocument();
+        },
+      );
+    });
+  });
 });

frontend/src/metabase/query_builder/components/filters/FilterPopover/FilterPopoverHeader.tsx

@@ -43,7 +43,7 @@ export default function FilterPopoverHeader({
 
   return showHeader ? (
     <div
-      className={cx(className, "text-medium p1 mb1 border-bottom", {
+      className={cx(className, "text-medium p1", {
         "flex align-center": !showOperatorSelectorOnOwnRow,
       })}
     >