Filters blocking small screen devices (#21848)

* ParameterWidget to have width of 100% on small devices

* Sticky filters only when there is less than 6

* stickyParameters tests updated

* Misc

* Misc

* Stickyness updated for small screen devices only

* Reverted unit test as we can’t test mobile/desktop screen size

* Misc

* describe updateParametersWidgetStickiness added

* Stickiness cypress tests WIP

* Tests clean up WIP

* Tests updated for 12 parameters

* Sticky filters test WIP

* data-testid added for ParametersWidgetContainer

* Percy clean up / parameters widget position checks

* data-testid added for ParametersWidgetContainer editing

* Parameters edit mode tests updated

* Improved stickiness performance

* Misc

frontend/src/metabase/dashboard/components/Dashboard/Dashboard.jsx

@@ -31,6 +31,7 @@ export default class Dashboard extends Component {
   state = {
     error: null,
     isParametersWidgetSticky: false,
+    parametersListLength: 0,
   };
 
   static propTypes = {
@@ -98,6 +99,12 @@ export default class Dashboard extends Component {
     this.parametersAndCardsContainerRef = React.createRef();
   }
 
+  static getDerivedStateFromProps(props, state) {
+    return props.parameters.length !== state.parametersListLength
+      ? { parametersListLength: props.parameters.length }
+      : null;
+  }
+
   throttleParameterWidgetStickiness = _.throttle(
     () => updateParametersWidgetStickiness(this),
     SCROLL_THROTTLE_INTERVAL,
@@ -206,9 +213,9 @@ export default class Dashboard extends Component {
       isNightMode,
       isSharing,
       parameters,
+      parameterValues,
       isNavbarOpen,
       showAddQuestionSidebar,
-      parameterValues,
       editingParameter,
       setParameterValue,
       setParameterIndex,
@@ -270,7 +277,10 @@ export default class Dashboard extends Component {
               />
 
               {shouldRenderParametersWidgetInEditMode && (
-                <ParametersWidgetContainer isEditing={isEditing}>
+                <ParametersWidgetContainer
+                  data-testid="edit-dashboard-parameters-widget-container"
+                  isEditing={isEditing}
+                >
                   {parametersWidget}
                 </ParametersWidgetContainer>
               )}
@@ -283,6 +293,7 @@ export default class Dashboard extends Component {
               >
                 {shouldRenderParametersWidgetInViewMode && (
                   <ParametersWidgetContainer
+                    data-testid="dashboard-parameters-widget-container"
                     ref={element => (this.parametersWidgetRef = element)}
                     isNavbarOpen={isNavbarOpen}
                     isSticky={isParametersWidgetSticky}

frontend/src/metabase/dashboard/components/Dashboard/stickyParameters.js

-import { getMainElement } from "metabase/lib/dom";
+import { isSmallScreen, getMainElement } from "metabase/lib/dom";
+
+export const MAXIMUM_PARAMETERS_FOR_STICKINESS = 6;
 
 export const updateParametersWidgetStickiness = dashboard => {
   initializeWidgetOffsetTop(dashboard);
@@ -34,11 +36,15 @@ const checkIfShouldToggleStickiness = (dashboard, shouldBeSticky) => {
 };
 
 const checkIfParametersWidgetShouldBeSticky = dashboard => {
+  const isStickyForDevice = !(
+    dashboard.state.parametersListLength > MAXIMUM_PARAMETERS_FOR_STICKINESS &&
+    isSmallScreen()
+  );
   const offsetTop =
     dashboard.state.parametersWidgetOffsetTop ||
     dashboard.parametersWidgetRef.offsetTop;
 
-  return getMainElement().scrollTop >= offsetTop;
+  return isStickyForDevice && getMainElement().scrollTop >= offsetTop;
 };
 
 const updateParametersAndCardsContainerStyle = (dashboard, shouldBeSticky) => {

frontend/src/metabase/dashboard/components/Dashboard/stickyParameters.unit.spec.js

 import { updateParametersWidgetStickiness } from "./stickyParameters";
 
+const offsetTop = 100;
+
 function mockMainElementScroll(scrollTop) {
   const fakeMainElement = { scrollTop };
   document.getElementsByTagName = () => [fakeMainElement];
 }
 
-it("initializes parametersWidgetOffsetTop", () => {
-  const offsetTop = 100;
-  const setState = jest.fn();
+describe("updateParametersWidgetStickiness", () => {
+  it("initializes parametersWidgetOffsetTop", () => {
+    const setState = jest.fn();
 
-  mockMainElementScroll(0);
+    mockMainElementScroll(0);
 
-  const dashboard = {
-    parametersWidgetRef: { offsetTop },
-    parametersAndCardsContainerRef: { style: {} },
-    state: {},
-    setState,
-  };
+    const dashboard = {
+      parametersWidgetRef: { offsetTop },
+      parametersAndCardsContainerRef: { style: {} },
+      state: {},
+      setState,
+    };
 
-  updateParametersWidgetStickiness(dashboard);
+    updateParametersWidgetStickiness(dashboard);
 
-  expect(setState).toHaveBeenCalledWith({
-    parametersWidgetOffsetTop: offsetTop,
+    expect(setState).toHaveBeenCalledWith({
+      parametersWidgetOffsetTop: offsetTop,
+    });
   });
-});
 
-it("makes filters sticky with enough scrolling down", () => {
-  const offsetTop = 100;
-  const setState = jest.fn();
+  it("makes filters sticky with enough scrolling down", () => {
+    const setState = jest.fn();
 
-  mockMainElementScroll(offsetTop + 1);
+    mockMainElementScroll(offsetTop + 1);
 
-  const dashboard = {
-    parametersWidgetRef: { offsetTop },
-    parametersAndCardsContainerRef: { style: {} },
-    state: {},
-    setState,
-  };
+    const dashboard = {
+      parametersWidgetRef: { offsetTop },
+      parametersAndCardsContainerRef: { style: {} },
+      state: {},
+      setState,
+    };
 
-  updateParametersWidgetStickiness(dashboard);
+    updateParametersWidgetStickiness(dashboard);
 
-  expect(setState).toHaveBeenCalledWith({
-    isParametersWidgetSticky: true,
+    expect(setState).toHaveBeenCalledWith({
+      isParametersWidgetSticky: true,
+    });
   });
-});
 
-it("makes filters unsticky with enough scrolling up", () => {
-  const offsetTop = 100;
-  const setState = jest.fn();
+  it("makes filters unsticky with enough scrolling up", () => {
+    const setState = jest.fn();
 
-  mockMainElementScroll(offsetTop - 1);
+    mockMainElementScroll(offsetTop - 1);
 
-  const dashboard = {
-    parametersWidgetRef: { offsetTop },
-    parametersAndCardsContainerRef: { style: {} },
-    state: {},
-    setState,
-  };
+    const dashboard = {
+      parametersWidgetRef: { offsetTop },
+      parametersAndCardsContainerRef: { style: {} },
+      state: {},
+      setState,
+    };
 
-  updateParametersWidgetStickiness(dashboard);
+    updateParametersWidgetStickiness(dashboard);
 
-  expect(setState).toHaveBeenCalledWith({
-    isParametersWidgetSticky: false,
+    expect(setState).toHaveBeenCalledWith({
+      isParametersWidgetSticky: false,
+    });
   });
-});
 
-it("keeps filters sticky with enough scrolling down and already sticky", () => {
-  const offsetTop = 100;
-  const setState = jest.fn();
+  it("keeps filters sticky with enough scrolling down and already sticky", () => {
+    const setState = jest.fn();
 
-  mockMainElementScroll(offsetTop + 1);
+    mockMainElementScroll(offsetTop + 1);
 
-  const dashboard = {
-    parametersWidgetRef: { offsetTop },
-    parametersAndCardsContainerRef: { style: {} },
-    state: { isParametersWidgetSticky: true, parametersWidgetOffsetTop: 100 },
-    setState,
-  };
+    const dashboard = {
+      parametersWidgetRef: { offsetTop },
+      parametersAndCardsContainerRef: { style: {} },
+      state: {
+        isParametersWidgetSticky: true,
+        parametersWidgetOffsetTop: offsetTop,
+      },
+      setState,
+    };
 
-  updateParametersWidgetStickiness(dashboard);
+    updateParametersWidgetStickiness(dashboard);
 
-  expect(setState).not.toHaveBeenCalled();
-});
+    expect(setState).not.toHaveBeenCalled();
+  });
 
-it("keeps filters not sticky with enough scrolling up and already not sticky", () => {
-  const offsetTop = 100;
-  const setState = jest.fn();
+  it("keeps filters not sticky with enough scrolling up and already not sticky", () => {
+    const setState = jest.fn();
 
-  mockMainElementScroll(offsetTop - 1);
+    mockMainElementScroll(offsetTop - 1);
 
-  const dashboard = {
-    parametersWidgetRef: { offsetTop },
-    parametersAndCardsContainerRef: { style: {} },
-    state: { isParametersWidgetSticky: false, parametersWidgetOffsetTop: 100 },
-    setState,
-  };
+    const dashboard = {
+      parametersWidgetRef: { offsetTop },
+      parametersAndCardsContainerRef: { style: {} },
+      state: {
+        isParametersWidgetSticky: false,
+        parametersWidgetOffsetTop: offsetTop,
+      },
+      setState,
+    };
 
-  updateParametersWidgetStickiness(dashboard);
+    updateParametersWidgetStickiness(dashboard);
 
-  expect(setState).not.toHaveBeenCalled();
+    expect(setState).not.toHaveBeenCalled();
+  });
 });

frontend/src/metabase/parameters/components/ParameterWidget.css

@@ -2,9 +2,16 @@
   composes: flex align-center from "style";
   transition: opacity 500ms linear;
   border: 2px solid var(--color-border);
-  margin-right: 0.85em;
-  margin-bottom: 0.5em;
-  padding: 0.25em 1em 0.25em 1em;
+  margin: 0 0 0.5em 0;
+  padding: 0.25em 1em;
+  width: 100%;
+}
+
+@media screen and (--breakpoint-min-sm) {
+  :local(.container) {
+    margin-right: 0.85em;
+    width: auto;
+  }
 }
 
 :local(.container) legend {

frontend/test/metabase-visual/dashboard/parameters-widget.cy.spec.js

@@ -7,26 +7,29 @@ const questionDetails = {
   query: { "source-table": PRODUCTS_ID },
 };
 
-const filter = {
+const parameter = {
   name: "Category",
   slug: "category",
   id: "ad1c877e",
   type: "category",
 };
 
-const filters = new Array(12).fill(filter);
+describe(`visual tests > dashboard > parameters widget`, () => {
+  const parametersShort = new Array(5).fill(parameter);
+  const parametersLong = new Array(12).fill(parameter);
 
-describe("visual tests > dashboard > parameters widget", () => {
-  beforeEach(() => {
-    restore();
-    cy.signInAsAdmin();
+  describe(`${parametersShort.length} filters (sticky on mobile)`, () => {
+    beforeEach(() => {
+      restore();
+      cy.signInAsAdmin();
 
-    cy.createQuestionAndDashboard({ questionDetails }).then(
-      ({ body: card }) => {
+      cy.createQuestionAndDashboard({
+        questionDetails,
+      }).then(({ body: card }) => {
         const { dashboard_id } = card;
 
         cy.request("PUT", `/api/dashboard/${dashboard_id}`, {
-          parameters: filters,
+          parameters: parametersShort,
         });
 
         const updatedSize = {
@@ -37,27 +40,185 @@ describe("visual tests > dashboard > parameters widget", () => {
         cy.editDashboardCard(card, updatedSize);
 
         visitDashboard(dashboard_id);
-      },
-    );
-  });
+      });
+    });
+
+    describe(`desktop`, () => {
+      it("is sticky in view mode", () => {
+        cy.findByText("test question");
+
+        cy.get("main")
+          .scrollTo(0, 264)
+          .then(() => {
+            cy.percySnapshot();
+          });
+
+        cy.findByTestId("dashboard-parameters-widget-container").should(
+          "have.css",
+          "position",
+          "fixed",
+        );
+      });
+
+      it("is sticky in edit mode", () => {
+        cy.findByText("test question");
+
+        cy.icon("pencil").click();
+
+        cy.findByTestId("dashboard-parameters-and-cards")
+          .scrollTo(0, 464)
+          .then(() => {
+            cy.percySnapshot();
+          });
 
-  it("is sticky in view mode", () => {
-    cy.findByText("test question");
+        cy.findByTestId("edit-dashboard-parameters-widget-container").should(
+          "not.have.css",
+          "position",
+          "fixed",
+        );
+      });
+    });
+
+    describe(`mobile`, () => {
+      it("is sticky in view mode", () => {
+        cy.findByText("test question");
+
+        cy.viewport(375, 667); // iPhone SE
+
+        cy.get("main")
+          .scrollTo(0, 264)
+          .then(() => {
+            cy.percySnapshot();
+          });
+
+        cy.findByTestId("dashboard-parameters-widget-container").should(
+          "have.css",
+          "position",
+          "fixed",
+        );
+      });
 
-    cy.get("main").scrollTo(0, 264);
+      it("is sticky in edit mode", () => {
+        cy.findByText("test question");
 
-    cy.percySnapshot();
+        cy.viewport(375, 667); // iPhone SE
+
+        cy.icon("pencil").click();
+
+        cy.findByTestId("dashboard-parameters-and-cards")
+          .scrollTo(0, 464)
+          .then(() => {
+            cy.percySnapshot();
+          });
+
+        cy.findByTestId("edit-dashboard-parameters-widget-container").should(
+          "not.have.css",
+          "position",
+          "fixed",
+        );
+      });
+    });
   });
 
-  it("is sticky in edit mode", () => {
-    cy.findByText("test question");
+  describe(`${parametersLong.length} filters (non sticky on mobile)`, () => {
+    beforeEach(() => {
+      restore();
+      cy.signInAsAdmin();
+
+      cy.createQuestionAndDashboard({
+        questionDetails,
+      }).then(({ body: card }) => {
+        const { dashboard_id } = card;
+
+        cy.request("PUT", `/api/dashboard/${dashboard_id}`, {
+          parameters: parametersLong,
+        });
+
+        const updatedSize = {
+          sizeX: 12,
+          sizeY: 32,
+        };
+
+        cy.editDashboardCard(card, updatedSize);
+
+        visitDashboard(dashboard_id);
+      });
+    });
+
+    describe(`desktop`, () => {
+      it("is sticky in view mode", () => {
+        cy.findByText("test question");
+
+        cy.get("main")
+          .scrollTo(0, 264)
+          .then(() => {
+            cy.percySnapshot();
+          });
+
+        cy.findByTestId("dashboard-parameters-widget-container").should(
+          "have.css",
+          "position",
+          "fixed",
+        );
+      });
+
+      it("is sticky in edit mode", () => {
+        cy.findByText("test question");
+
+        cy.icon("pencil").click();
+
+        cy.findByTestId("dashboard-parameters-and-cards")
+          .scrollTo(0, 464)
+          .then(() => {
+            cy.percySnapshot();
+          });
+
+        cy.findByTestId("edit-dashboard-parameters-widget-container").should(
+          "not.have.css",
+          "position",
+          "fixed",
+        );
+      });
+    });
+
+    describe(`mobile`, () => {
+      it("is not sticky in view mode", () => {
+        cy.findByText("test question");
+
+        cy.viewport(375, 667); // iPhone SE
+
+        cy.get("main")
+          .scrollTo(0, 264)
+          .then(() => {
+            cy.percySnapshot();
+          });
+
+        cy.findByTestId("dashboard-parameters-widget-container").should(
+          "not.have.css",
+          "position",
+          "fixed",
+        );
+      });
+
+      it("is not sticky in edit mode", () => {
+        cy.findByText("test question");
+
+        cy.viewport(375, 667); // iPhone SE
+
+        cy.icon("pencil").click();
 
-    cy.icon("pencil").click();
+        cy.findByTestId("dashboard-parameters-and-cards")
+          .scrollTo(0, 464)
+          .then(() => {
+            cy.percySnapshot();
+          });
 
-    cy.findByTestId("dashboard-parameters-and-cards")
-      .scrollTo(0, 464)
-      .then(() => {
-        cy.percySnapshot();
+        cy.findByTestId("edit-dashboard-parameters-widget-container").should(
+          "not.have.css",
+          "position",
+          "fixed",
+        );
       });
+    });
   });
 });