Collapse temporal units list (#32953)

* Collapse temporal units list

* Extract `MoreButton` component

* Simplify conditions

* Make `isInitiallyExpanded` a bit more clear

* Use `INITIALLY_VISIBLE_ITEMS_COUNT` in tests

* Fix typos

* Remove comment

* Update popover size and spacing

* Remove outdated test

* Update E2E tests

* Add hover state to the "More" button

* Disable hover transition

e2e/test/scenarios/binning/binning-options.cy.spec.js

@@ -120,7 +120,11 @@ describe("scenarios > binning > binning options", () => {
       getTitle("Count by Created At: Month");
 
       openBinningListForDimension("Created At", "by month");
-      getAllOptions({ options: TIME_BUCKETS, isSelected: "Month" });
+      getAllOptions({
+        options: TIME_BUCKETS,
+        isSelected: "Month",
+        shouldExpandList: true,
+      });
     });
 
     it("should render longitude/latitude binning options correctly", () => {
@@ -162,7 +166,11 @@ describe("scenarios > binning > binning options", () => {
       cy.findByText("Created At: Month").click();
       openBinningListForDimension("Created At", "by month");
 
-      getAllOptions({ options: TIME_BUCKETS, isSelected: "Month" });
+      getAllOptions({
+        options: TIME_BUCKETS,
+        isSelected: "Month",
+        shouldExpandList: true,
+      });
     });
 
     it("should render longitude/latitude binning options correctly", () => {
@@ -305,7 +313,7 @@ function getTitle(title) {
   cy.findByText(title);
 }
 
-function getAllOptions({ options, isSelected } = {}) {
+function getAllOptions({ options, isSelected, shouldExpandList } = {}) {
   const selectedOption = options.find(option => option === isSelected);
   const regularOptions = options.filter(option => option !== isSelected);
 
@@ -315,6 +323,10 @@ function getAllOptions({ options, isSelected } = {}) {
   popover()
     .last()
     .within(() => {
+      if (shouldExpandList) {
+        cy.button("More…").click();
+      }
+
       regularOptions.forEach(option => {
         // Implicit assertion - will fail if string is rendered multiple times
         cy.findByText(option);

e2e/test/scenarios/binning/binning-reproductions.cy.spec.js

@@ -9,6 +9,7 @@ import {
   summarize,
   openOrdersTable,
   getNotebookStep,
+  rightSidebar,
 } from "e2e/support/helpers";
 
 import { SAMPLE_DB_ID } from "e2e/support/cypress_data";
@@ -180,8 +181,7 @@ describe("binning related reproductions", () => {
     cy.get("circle");
   });
 
-  it("should display date granularity on Summarize when opened from saved question (metabase#11439)", () => {
-    // save "Orders" as question
+  it("should display date granularity on Summarize when opened from saved question (metabase#10441, metabase#11439)", () => {
     cy.createQuestion({
       name: "11439",
       query: { "source-table": ORDERS_ID },
@@ -190,34 +190,24 @@ describe("binning related reproductions", () => {
     // it is essential for this repro to find question following these exact steps
     // (for example, visiting `/collection/root` would yield different result)
     startNewQuestion();
-    // eslint-disable-next-line no-unscoped-text-selectors -- deprecated usage
-    cy.findByText("Saved Questions").click();
-    // eslint-disable-next-line no-unscoped-text-selectors -- deprecated usage
-    cy.findByText("11439").click();
-    visualize();
+    popover().within(() => {
+      cy.findByText("Saved Questions").click();
+      cy.findByText("11439").click();
+    });
 
+    visualize();
     summarize();
 
-    // eslint-disable-next-line no-unscoped-text-selectors -- deprecated usage
-    cy.findByText("Group by")
-      .parent()
-      .within(() => {
-        cy.log("Reported failing since v0.33.5.1");
-        cy.log(
-          "**Marked as regression of [#10441](https://github.com/metabase/metabase/issues/10441)**",
-        );
-
-        cy.findAllByText("Created At")
-          .eq(0)
-          .closest("li")
-          .contains("by month")
-          // realHover() or mousemove don't work for whatever reason
-          // have to use this ugly hack for now
-          .click({ force: true });
-      });
-    // // this step is maybe redundant since it fails to even find "by month"
-    // eslint-disable-next-line no-unscoped-text-selectors -- deprecated usage
-    cy.findByText("Hour of day");
+    rightSidebar().within(() => {
+      cy.findByRole("listitem", { name: "Created At" })
+        .findByLabelText("Temporal bucket")
+        .click();
+    });
+
+    popover().within(() => {
+      cy.button("More…").click();
+      cy.findByText("Hour of day").should("exist");
+    });
   });
 
   it("shouldn't duplicate the breakout field (metabase#22382)", () => {

e2e/test/scenarios/binning/correctness/shared/constants.js

@@ -34,34 +34,42 @@ export const TIME_OPTIONS = {
     selected: "by minute of hour",
     representativeValues: ["0", "5", "8", "13"],
     type: "extended",
+    isHiddenByDefault: true,
   },
   "Hour of day": {
     selected: "by hour of day",
     representativeValues: ["12:00 AM", "2:00 AM", "12:00 PM", "8:00 PM"],
+    isHiddenByDefault: true,
   },
   "Day of week": {
     selected: "by day of week",
     representativeValues: ["Saturday", "Tuesday", "Friday", "Sunday"],
+    isHiddenByDefault: true,
   },
   "Day of month": {
     selected: "by day of month",
     representativeValues: ["5", "10", "15", "30"],
+    isHiddenByDefault: true,
   },
   "Day of year": {
     selected: "by day of year",
     representativeValues: ["1", "10", "12"],
+    isHiddenByDefault: true,
   },
   "Week of year": {
     selected: "by week of year",
     representativeValues: ["1st", "2nd", "3rd", "10th"],
+    isHiddenByDefault: true,
   },
   "Month of year": {
     selected: "by month of year",
     representativeValues: ["January", "June", "December"],
+    isHiddenByDefault: true,
   },
   "Quarter of year": {
     selected: "by quarter of year",
     representativeValues: ["Q1", "Q2", "Q3", "Q4"],
+    isHiddenByDefault: true,
   },
 };
 

e2e/test/scenarios/binning/correctness/time-series.cy.spec.js

@@ -3,6 +3,7 @@ import {
   popover,
   getBinningButtonForDimension,
   summarize,
+  rightSidebar,
 } from "e2e/support/helpers";
 
 import { SAMPLE_DATABASE } from "e2e/support/cypress_sample_database";
@@ -38,9 +39,12 @@ describe("scenarios > binning > correctness > time series", () => {
   });
 
   Object.entries(TIME_OPTIONS).forEach(
-    ([bucketSize, { selected, representativeValues }]) => {
+    ([bucketSize, { selected, isHiddenByDefault, representativeValues }]) => {
       it(`should return correct values for ${bucketSize}`, () => {
         popover().within(() => {
+          if (isHiddenByDefault) {
+            cy.button("More…").click();
+          }
           cy.findByText(bucketSize).click();
           cy.wait("@dataset");
         });
@@ -50,8 +54,7 @@ describe("scenarios > binning > correctness > time series", () => {
           isSelected: true,
         }).should("have.text", selected);
 
-        // eslint-disable-next-line no-unscoped-text-selectors -- deprecated usage
-        cy.findByText("Done").click();
+        rightSidebar().button("Done").click();
 
         getTitle(`Count by Created At: ${bucketSize}`);
 

e2e/test/scenarios/question/summarization.cy.spec.js

@@ -10,7 +10,6 @@ import {
   openOrdersTable,
   enterCustomColumnDetails,
   visualize,
-  getNotebookStep,
   checkExpressionEditorHelperPopoverPosition,
 } from "e2e/support/helpers";
 
@@ -152,32 +151,6 @@ describe("scenarios > question > summarize sidebar", () => {
     cy.findByText("49.54");
   });
 
-  it("breakout binning popover should have normal height even when it's rendered lower on the screen (metabase#15445)", () => {
-    visitQuestion(ORDERS_QUESTION_ID);
-    cy.icon("notebook").click();
-
-    summarize({ mode: "notebook" });
-    popover().findByText("Count of rows").click();
-
-    getNotebookStep("summarize")
-      .findByText("Pick a column to group by")
-      .click();
-    popover()
-      .findByRole("option", { name: "Created At" })
-      .realHover()
-      .findByLabelText("Temporal bucket")
-      .findByText("by month")
-      .click();
-
-    cy.findByRole("tooltip").within(() => {
-      cy.findByText("Minute").should("be.visible");
-      cy.findByText("Week").should("be.visible");
-
-      // Ensure the option is there, but not visible (have to scroll the list to see it)
-      cy.findByText("Quarter of year").should("exist").should("not.be.visible");
-    });
-  });
-
   it("should allow using `Custom Expression` in orders metrics (metabase#12899)", () => {
     openOrdersTable({ mode: "notebook" });
     summarize({ mode: "notebook" });
@@ -234,23 +207,20 @@ describe("scenarios > question > summarize sidebar", () => {
   });
 
   it("summarizing by distinct datetime should allow granular selection (metabase#13098)", () => {
-    // Go straight to orders table in custom questions
     openOrdersTable({ mode: "notebook" });
 
     summarize({ mode: "notebook" });
     popover().within(() => {
       cy.findByText("Number of distinct values of ...").click();
-      cy.log(
-        "**Test fails at this point as there isn't an extra field next to 'Created At'**",
-      );
-      // instead of relying on DOM structure that might change
-      // (i.e. find "Created At" -> parent -> parent -> parent -> find "by month")
-      // access it directly from the known common parent
-      cy.get(".List-item").contains("by month").click({ force: true });
+      cy.findByLabelText("Temporal bucket").click();
     });
-    // this should be among the granular selection choices
-    // eslint-disable-next-line no-unscoped-text-selectors -- deprecated usage
-    cy.findByText("Hour of day").click();
+
+    popover()
+      .last()
+      .within(() => {
+        cy.button("More…").click();
+        cy.findByText("Hour of day").click();
+      });
   });
 
   it.skip("should handle (removing) multiple metrics when one is sorted (metabase#12625)", () => {

frontend/src/metabase/common/components/QueryColumnPicker/BucketPickerPopover/BaseBucketPickerPopover.styled.tsx

 import styled from "@emotion/styled";
+import Button from "metabase/core/components/Button";
 import { Icon } from "metabase/core/components/Icon";
 import BaseSelectList from "metabase/components/SelectList";
 import { alpha, color } from "metabase/lib/colors";
@@ -39,12 +40,30 @@ export const SelectListItem = styled(BaseSelectList.Item)<{
   }
 `;
 
-export const SelectList = styled(BaseSelectList)`
+export const Content = styled.div`
   overflow-y: auto;
-  max-height: 390px;
-  padding: 0.5rem 1rem;
+  padding: 0.5rem;
+  min-width: 160px;
 
   ${SelectListItem} {
     margin: 2px 0;
   }
 `;
+
+export const MoreButton = styled(Button)`
+  width: 100%;
+  height: 36px;
+  padding: 8px 16px;
+
+  transition: none !important;
+
+  ${Button.Content} {
+    justify-content: flex-start;
+  }
+
+  &:hover {
+    background-color: ${color("brand-lighter")};
+  }
+`;
+
+MoreButton.defaultProps = { onlyText: true };

frontend/src/metabase/common/components/QueryColumnPicker/BucketPickerPopover/BaseBucketPickerPopover.tsx

-import { useMemo } from "react";
+import { useCallback, useMemo, useState } from "react";
+import { t } from "ttag";
+import SelectList from "metabase/components/SelectList";
 import PopoverWithTrigger from "metabase/components/PopoverWithTrigger/TippyPopoverWithTrigger";
 import type { ColorName } from "metabase/lib/colors/types";
 import * as Lib from "metabase-lib";
 import {
+  Content,
+  MoreButton,
   TriggerButton,
   TriggerIcon,
-  SelectList,
   SelectListItem,
 } from "./BaseBucketPickerPopover.styled";
 
+export const INITIALLY_VISIBLE_ITEMS_COUNT = 7;
+
 type NoBucket = null;
 
 export type BucketListItem = Lib.BucketDisplayInfo & {
@@ -42,16 +47,39 @@ function _BaseBucketPickerPopover({
   renderTriggerContent,
   onSelect,
 }: BaseBucketPickerPopoverProps) {
+  const [isExpanded, setIsExpanded] = useState(
+    isInitiallyExpanded(items, selectedBucket, checkBucketIsSelected),
+  );
+
   const defaultBucket = useMemo(
     () => items.find(item => item.default)?.bucket,
     [items],
   );
 
+  const handleExpand = useCallback(() => {
+    setIsExpanded(true);
+  }, []);
+
+  const handlePopoverClose = useCallback(() => {
+    const nextState = isInitiallyExpanded(
+      items,
+      selectedBucket,
+      checkBucketIsSelected,
+    );
+    setIsExpanded(nextState);
+  }, [items, selectedBucket, checkBucketIsSelected]);
+
   const triggerContentBucket = isEditing ? selectedBucket : defaultBucket;
   const triggerContentBucketDisplayInfo = triggerContentBucket
     ? Lib.displayInfo(query, stageIndex, triggerContentBucket)
     : undefined;
 
+  const canExpand = items.length > INITIALLY_VISIBLE_ITEMS_COUNT;
+  const hasMoreButton = canExpand && !isExpanded;
+  const visibleItems = hasMoreButton
+    ? items.slice(0, INITIALLY_VISIBLE_ITEMS_COUNT)
+    : items;
+
   return (
     <PopoverWithTrigger
       renderTrigger={({ onClick }) => (
@@ -70,26 +98,49 @@ function _BaseBucketPickerPopover({
         </TriggerButton>
       )}
       popoverContent={({ closePopover }) => (
-        <SelectList>
-          {items.map(item => (
-            <SelectListItem
-              id={item.displayName}
-              key={item.displayName}
-              name={item.displayName}
-              activeColor={color}
-              isSelected={checkBucketIsSelected(item)}
-              onSelect={() => {
-                onSelect(item.bucket);
-                closePopover();
-              }}
-            />
-          ))}
-        </SelectList>
+        <Content>
+          <SelectList>
+            {visibleItems.map(item => (
+              <SelectListItem
+                id={item.displayName}
+                key={item.displayName}
+                name={item.displayName}
+                activeColor={color}
+                isSelected={checkBucketIsSelected(item)}
+                onSelect={() => {
+                  onSelect(item.bucket);
+                  closePopover();
+                }}
+              />
+            ))}
+          </SelectList>
+          {hasMoreButton && (
+            <MoreButton onClick={handleExpand}>{t`More…`}</MoreButton>
+          )}
+        </Content>
       )}
+      onClose={handlePopoverClose}
     />
   );
 }
 
+function isInitiallyExpanded(
+  items: BucketListItem[],
+  selectedBucket: Lib.Bucket | NoBucket,
+  checkBucketIsSelected: (item: BucketListItem) => boolean,
+) {
+  const canExpand = items.length > INITIALLY_VISIBLE_ITEMS_COUNT;
+  if (!canExpand || !selectedBucket) {
+    return false;
+  }
+
+  const isSelectedBucketAmongHiddenItems =
+    items.findIndex(item => checkBucketIsSelected(item)) >=
+    INITIALLY_VISIBLE_ITEMS_COUNT;
+
+  return isSelectedBucketAmongHiddenItems;
+}
+
 export function getBucketListItem(
   query: Lib.Query,
   stageIndex: number,

frontend/src/metabase/common/components/QueryColumnPicker/BucketPickerPopover/BucketPickerPopover.tsx

 import * as Lib from "metabase-lib";
-import { BaseBucketPickerPopover } from "./BaseBucketPickerPopover";
+import {
+  BaseBucketPickerPopover,
+  INITIALLY_VISIBLE_ITEMS_COUNT,
+} from "./BaseBucketPickerPopover";
 import { BinningStrategyPickerPopover } from "./BinningStrategyPickerPopover";
 import { TemporalBucketPickerPopover } from "./TemporalBucketPickerPopover";
 import type { CommonBucketPickerProps } from "./types";
@@ -53,6 +56,8 @@ function _BucketPickerPopover({
   return null;
 }
 
+export { INITIALLY_VISIBLE_ITEMS_COUNT };
+
 export const BucketPickerPopover = Object.assign(_BucketPickerPopover, {
   displayName: "BucketPickerPopover",
   TriggerButton: BaseBucketPickerPopover.TriggerButton,

frontend/src/metabase/common/components/QueryColumnPicker/BucketPickerPopover/BucketPickerPopover.unit.spec.tsx

+import userEvent from "@testing-library/user-event";
+import { render, screen, waitFor } from "__support__/ui";
+import * as Lib from "metabase-lib";
+import { createQuery, columnFinder } from "metabase-lib/test-helpers";
+import {
+  BucketPickerPopover,
+  INITIALLY_VISIBLE_ITEMS_COUNT,
+} from "./BucketPickerPopover";
+
+const query = createQuery();
+const findColumn = columnFinder(query, Lib.breakoutableColumns(query, 0));
+const dateColumn = findColumn("ORDERS", "CREATED_AT");
+const numericColumn = findColumn("ORDERS", "TOTAL");
+
+function setup({ column }: { column: Lib.ColumnMetadata }) {
+  const onSelect = jest.fn();
+  render(
+    <div data-testid="container">
+      <BucketPickerPopover
+        query={query}
+        stageIndex={0}
+        column={column}
+        isEditing
+        hasBinning
+        hasTemporalBucketing
+        onSelect={onSelect}
+      />
+    </div>,
+  );
+}
+
+async function setupBinningPicker({ column }: { column: Lib.ColumnMetadata }) {
+  setup({ column });
+  userEvent.click(screen.getByLabelText("Binning strategy"));
+  await screen.findByText("Don't bin");
+}
+
+async function setupTemporalBucketPicker({
+  column,
+}: {
+  column: Lib.ColumnMetadata;
+}) {
+  setup({ column });
+  userEvent.click(screen.getByLabelText("Temporal bucket"));
+  await screen.findByText("Year");
+}
+
+describe("BucketPickerPopover", () => {
+  it("should collapse long lists", async () => {
+    const buckets = Lib.availableTemporalBuckets(query, 0, dateColumn);
+    await setupTemporalBucketPicker({ column: dateColumn });
+
+    expect(screen.getAllByRole("menuitem")).toHaveLength(
+      INITIALLY_VISIBLE_ITEMS_COUNT,
+    );
+    expect(screen.getByText("Minute")).toBeInTheDocument();
+    expect(screen.getByText("Year")).toBeInTheDocument();
+
+    expect(screen.queryByText("Minute of hour")).not.toBeInTheDocument();
+    expect(screen.queryByText("Day of month")).not.toBeInTheDocument();
+    expect(screen.queryByText("Month of year")).not.toBeInTheDocument();
+
+    userEvent.click(screen.getByRole("button", { name: "More…" }));
+
+    expect(screen.getAllByRole("menuitem")).toHaveLength(buckets.length);
+  });
+
+  it("shouldn't show the More button if there are a few buckets", async () => {
+    const buckets = Lib.availableBinningStrategies(query, 0, numericColumn);
+    await setupBinningPicker({ column: numericColumn });
+
+    expect(screen.getAllByRole("menuitem")).toHaveLength(
+      ["Don't bin", ...buckets].length,
+    );
+    expect(screen.queryByText("More…")).not.toBeInTheDocument();
+  });
+
+  it("should expand the list if the selected bucket is in the hidden part", async () => {
+    const buckets = Lib.availableTemporalBuckets(query, 0, dateColumn);
+    const lastBucket = buckets[buckets.length - 1];
+    const column = Lib.withTemporalBucket(dateColumn, lastBucket);
+    await setupTemporalBucketPicker({ column });
+
+    expect(screen.getAllByRole("menuitem")).toHaveLength(buckets.length);
+    expect(screen.queryByText("More…")).not.toBeInTheDocument();
+  });
+
+  it("should collapse after popover is closed", async () => {
+    await setupTemporalBucketPicker({ column: dateColumn });
+
+    userEvent.click(screen.getByRole("button", { name: "More…" }));
+    // Clicking outside the popover should close it
+    userEvent.click(screen.getByTestId("container"));
+
+    await waitFor(() => expect(screen.getByText("Month")).not.toBeVisible());
+
+    userEvent.click(screen.getByLabelText("Temporal bucket"));
+    await screen.findByText("Month");
+
+    expect(screen.getAllByRole("menuitem")).toHaveLength(
+      INITIALLY_VISIBLE_ITEMS_COUNT,
+    );
+  });
+
+  it("shouldn't collapse after popover is closed if the selected bucket is in the hidden part", async () => {
+    const buckets = Lib.availableTemporalBuckets(query, 0, dateColumn);
+    const lastBucket = buckets[buckets.length - 1];
+    const column = Lib.withTemporalBucket(dateColumn, lastBucket);
+    await setupTemporalBucketPicker({ column });
+
+    // Clicking outside the popover should close it
+    userEvent.click(screen.getByTestId("container"));
+
+    await waitFor(() => expect(screen.getByText("Month")).not.toBeVisible());
+
+    userEvent.click(screen.getByLabelText("Temporal bucket"));
+    await screen.findByText("Month");
+
+    expect(screen.getAllByRole("menuitem")).toHaveLength(buckets.length);
+  });
+});

frontend/src/metabase/components/PopoverWithTrigger/TippyPopoverWithTrigger.tsx

@@ -12,29 +12,34 @@ export type TippyPopoverWithTriggerRef = {
 export type TippyPopoverWithTriggerProps = {
   isInitiallyVisible?: boolean;
   popoverRef?: RefObject<TippyPopoverWithTriggerRef>;
+  onClose?: () => void;
 } & Omit<ControlledPopoverWithTriggerProps, "visible" | "onClose" | "onOpen">;
 
 function UncontrolledPopoverWithTrigger({
   isInitiallyVisible,
   popoverRef,
+  onClose,
   ...props
 }: TippyPopoverWithTriggerProps) {
   const [visible, setVisible] = useState(isInitiallyVisible || false);
 
-  const onOpen = useCallback(() => setVisible(true), []);
-  const onClose = useCallback(() => setVisible(false), []);
+  const handleOpen = useCallback(() => setVisible(true), []);
+  const handleClose = useCallback(() => {
+    setVisible(false);
+    onClose?.();
+  }, [onClose]);
 
   useImperativeHandle(popoverRef, () => ({
-    open: onOpen,
-    close: onClose,
+    open: handleOpen,
+    close: handleClose,
   }));
 
   return (
     <ControlledPopoverWithTrigger
       {...props}
       visible={visible}
-      onOpen={onOpen}
-      onClose={onClose}
+      onOpen={handleOpen}
+      onClose={handleClose}
     />
   );
 }