Add drag and drop reordering of pinned items (#19966)

* onDrop unneeded

* basic impl of reordering pinned items

* split cards and dashboards again

* Improve pin drop target styling

* Fix card drop target weirdness

* brand-light border --> brand

* pull out separate styled drop target component

* replace style prop usage

* reduce the z-index

* Remove prop that triggers drill functionality

* Fix clobbering of pinned list on update

* Fix card queries on every rerender

* Add pin drop zone & split drop target components

* lint fix

* fix & add a few tests

* update pin drop zone styling to match sorting

frontend/src/metabase/collections/components/CollectionCardVisualization/CollectionCardVisualization.jsx

-import React from "react";
+import React, { useRef } from "react";
 import PropTypes from "prop-types";
 import _ from "underscore";
 
@@ -29,6 +29,8 @@ function CollectionCardVisualization({
   onCopy,
   onMove,
 }) {
+  const questionRef = useRef();
+
   return (
     <ItemLink to={item.getUrl()}>
       <VizCard flat>
@@ -40,9 +42,12 @@ function CollectionCardVisualization({
         />
         <Questions.Loader id={item.id}>
           {({ question: card }) => {
-            const question = new Question(card, metadata);
+            // reusing the initial question instance avoids triggering queries every time this component rerenders
+            questionRef.current =
+              questionRef.current || new Question(card, metadata);
+
             return (
-              <QuestionResultLoader question={question}>
+              <QuestionResultLoader question={questionRef.current}>
                 {({ loading, error, reload, rawSeries, results, result }) => {
                   const shouldShowLoader = loading && results == null;
                   const { errorMessage, errorIcon } = getErrorProps(
@@ -56,7 +61,6 @@ function CollectionCardVisualization({
                       noWrapper
                     >
                       <Visualization
-                        onChangeCardAndRun={_.noop}
                         isDashboard
                         showTitle
                         metadata={metadata}

frontend/src/metabase/collections/components/CollectionCardVisualization/CollectionCardVisualization.styled.jsx

@@ -3,6 +3,7 @@ import styled from "styled-components";
 import ActionMenu from "metabase/collections/components/ActionMenu";
 import Card from "metabase/components/Card";
 import { color } from "metabase/lib/colors";
+import { LegendLabel } from "metabase/visualizations/components/legend/LegendCaption.styled";
 
 const HEIGHT = 250;
 
@@ -26,5 +27,9 @@ export const VizCard = styled(Card)`
     ${HoverMenu} {
       visibility: visible;
     }
+
+    ${LegendLabel} {
+      color: ${color("brand")};
+    }
   }
 `;

frontend/src/metabase/collections/components/ItemsTable.jsx

 import React from "react";
 import PropTypes from "prop-types";
-import { t } from "ttag";
 import { Flex } from "grid-styled";
 
-import { color } from "metabase/lib/colors";
-
-import PinDropTarget from "metabase/containers/dnd/PinDropTarget";
-
 import { ANALYTICS_CONTEXT } from "metabase/collections/constants";
+import PinDropZone from "metabase/collections/components/PinDropZone";
 
 import BaseItemsTable from "./BaseItemsTable";
 
-function ItemsEmptyState() {
-  return (
-    <PinDropTarget pinIndex={null} hideUntilDrag margin={10}>
-      {({ hovered }) => (
-        <Flex
-          align="center"
-          justify="center"
-          py={2}
-          m={2}
-          color={hovered ? color("brand") : color("text-medium")}
-        >
-          {t`Drag here to un-pin`}
-        </Flex>
-      )}
-    </PinDropTarget>
-  );
-}
-
 Item.propTypes = {
   item: PropTypes.object.isRequired,
 };
@@ -55,13 +33,18 @@ function ItemsTable(props) {
   const { items } = props;
 
   if (items.length === 0) {
-    return <ItemsEmptyState />;
+    return (
+      <Flex className="relative" align="center" justify="center" p={4} m={2}>
+        <PinDropZone variant="unpin" />
+      </Flex>
+    );
   }
 
   return (
-    <PinDropTarget pinIndex={null}>
+    <div className="relative">
+      <PinDropZone variant="unpin" />
       <BaseItemsTable {...props} renderItem={Item} />
-    </PinDropTarget>
+    </div>
   );
 }
 

frontend/src/metabase/collections/components/PinDropZone/PinDropZone.styled.tsx

+import styled from "styled-components";
+
+import PinDropTarget from "metabase/containers/dnd/PinDropTarget";
+import { color } from "metabase/lib/colors";
+
+export type PinDropTargetProps = {
+  variant: "pin" | "unpin";
+  pinIndex: number | null;
+  hideUntilDrag: boolean;
+};
+
+export type PinDropTargetRenderArgs = PinDropTargetProps & {
+  hovered: boolean;
+  highlighted: boolean;
+};
+
+export const StyledPinDropTarget = styled(PinDropTarget)<PinDropTargetProps>`
+  position: absolute !important;
+  top: 0;
+  bottom: 0;
+  left: -1rem;
+  right: -1rem;
+  pointer-events: none;
+  background-color: transparent !important;
+
+  * {
+    pointer-events: all;
+    background-color: transparent !important;
+  }
+`;
+
+export const PinDropTargetIndicator = styled.div<PinDropTargetRenderArgs>`
+  z-index: 1;
+  position: absolute;
+  top: 0;
+  bottom: 0;
+  left: 0;
+  right: 0;
+  border-left: ${props =>
+    `4px solid ${props.hovered ? color("brand") : color("bg-medium")}`};
+  display: ${props => !(props.hovered || props.highlighted) && "none"};
+`;

frontend/src/metabase/collections/components/PinDropZone/PinDropZone.tsx

+import React from "react";
+import PropTypes from "prop-types";
+
+import {
+  StyledPinDropTarget,
+  PinDropTargetIndicator,
+  PinDropTargetProps,
+  PinDropTargetRenderArgs,
+} from "./PinDropZone.styled";
+
+type PinDropZoneProps = Pick<PinDropTargetProps, "variant">;
+
+PinDropZone.propTypes = {
+  variant: PropTypes.oneOf(["pin", "unpin"]).isRequired,
+};
+
+function PinDropZone({ variant, ...props }: PinDropZoneProps) {
+  return (
+    <StyledPinDropTarget
+      variant={variant}
+      pinIndex={variant === "pin" ? 1 : null}
+      hideUntilDrag
+      {...props}
+    >
+      {(args: PinDropTargetRenderArgs) => <PinDropTargetIndicator {...args} />}
+    </StyledPinDropTarget>
+  );
+}
+
+export default PinDropZone;

frontend/src/metabase/collections/components/PinDropZone/index.ts

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

frontend/src/metabase/collections/components/PinnedItemOverview/PinnedItemOverview.styled.tsx

@@ -3,16 +3,19 @@ import styled from "styled-components";
 import { color } from "metabase/lib/colors";
 import { breakpointMaxMedium } from "metabase/styled-components/theme";
 
+export const GAP_REM = 1.15;
+
 export const Container = styled.div`
+  position: relative;
   display: flex;
   flex-direction: column;
-  gap: 1.15rem;
+  gap: ${GAP_REM}rem;
 `;
 
 export const Grid = styled.div`
   display: grid;
   grid-template-columns: minmax(0, 1fr) minmax(0, 1fr);
-  gap: 1.15rem;
+  gap: ${GAP_REM}rem;
 
   ${breakpointMaxMedium} {
     grid-template-columns: minmax(0, 1fr);

frontend/src/metabase/collections/components/PinnedItemOverview/PinnedItemOverview.tsx

@@ -6,7 +6,9 @@ import Metadata from "metabase-lib/lib/metadata/Metadata";
 import PinnedItemCard from "metabase/collections/components/PinnedItemCard";
 import CollectionCardVisualization from "metabase/collections/components/CollectionCardVisualization";
 import EmptyPinnedItemsBanner from "../EmptyPinnedItemsBanner/EmptyPinnedItemsBanner";
+import PinnedItemSortDropTarget from "metabase/collections/components/PinnedItemSortDropTarget";
 import { Item, Collection, isRootCollection } from "metabase/collections/utils";
+import PinDropZone from "metabase/collections/components/PinDropZone";
 import ItemDragSource from "metabase/containers/dnd/ItemDragSource";
 
 import {
@@ -22,7 +24,6 @@ type Props = {
   metadata: Metadata;
   onCopy: (items: Item[]) => void;
   onMove: (items: Item[]) => void;
-  onDrop: () => void;
 };
 
 function PinnedItemOverview({
@@ -31,9 +32,8 @@ function PinnedItemOverview({
   metadata,
   onCopy,
   onMove,
-  onDrop,
 }: Props) {
-  const sortedItems = _.sortBy(items, item => item.name);
+  const sortedItems = _.sortBy(items, item => item.collection_position);
   const {
     card: cardItems = [],
     dashboard: dashboardItems = [],
@@ -42,53 +42,75 @@ function PinnedItemOverview({
 
   return items.length === 0 ? (
     <Container>
+      <PinDropZone variant="pin" />
       <EmptyPinnedItemsBanner />
     </Container>
   ) : (
     <Container data-testid="pinned-items">
+      <PinDropZone variant="pin" />
       {cardItems.length > 0 && (
         <Grid>
           {cardItems.map(item => (
-            <ItemDragSource
-              key={item.id}
-              item={item}
-              collection={collection}
-              onDrop={onDrop}
-            >
-              <div>
-                <CollectionCardVisualization
-                  item={item}
-                  collection={collection}
-                  metadata={metadata}
-                  onCopy={onCopy}
-                  onMove={onMove}
-                />
-              </div>
-            </ItemDragSource>
+            <div key={item.id} className="relative">
+              <PinnedItemSortDropTarget
+                isFrontTarget
+                itemModel="card"
+                pinIndex={item.collection_position}
+                enableDropTargetBackground={false}
+              />
+              <ItemDragSource item={item} collection={collection}>
+                <div>
+                  <CollectionCardVisualization
+                    item={item}
+                    collection={collection}
+                    metadata={metadata}
+                    onCopy={onCopy}
+                    onMove={onMove}
+                  />
+                </div>
+              </ItemDragSource>
+              <PinnedItemSortDropTarget
+                isBackTarget
+                itemModel="card"
+                pinIndex={item.collection_position}
+                enableDropTargetBackground={false}
+              />
+            </div>
           ))}
         </Grid>
       )}
+
       {dashboardItems.length > 0 && (
         <Grid>
           {dashboardItems.map(item => (
-            <ItemDragSource
-              key={item.id}
-              item={item}
-              collection={collection}
-              onDrop={onDrop}
-            >
-              <div>
-                <PinnedItemCard
-                  item={item}
-                  collection={collection}
-                  onCopy={onCopy}
-                  onMove={onMove}
-                />
-              </div>
-            </ItemDragSource>
+            <div key={item.id} className="relative">
+              <PinnedItemSortDropTarget
+                isFrontTarget
+                itemModel="dashboard"
+                pinIndex={item.collection_position}
+                enableDropTargetBackground={false}
+              />
+              <ItemDragSource item={item} collection={collection}>
+                <div>
+                  <PinnedItemCard
+                    item={item}
+                    collection={collection}
+                    onCopy={onCopy}
+                    onMove={onMove}
+                  />
+                </div>
+              </ItemDragSource>
+              <PinnedItemSortDropTarget
+                isBackTarget
+                itemModel="dashboard"
+                pinIndex={item.collection_position}
+                enableDropTargetBackground={false}
+              />
+            </div>
           ))}
         </Grid>
       )}
+
       {dataModelItems.length > 0 && (
         <div>
           <SectionHeader>
@@ -101,21 +123,30 @@ function PinnedItemOverview({
           </SectionHeader>
           <Grid>
             {dataModelItems.map(item => (
-              <ItemDragSource
-                key={item.id}
-                item={item}
-                collection={collection}
-                onDrop={onDrop}
-              >
-                <div>
-                  <PinnedItemCard
-                    item={item}
-                    collection={collection}
-                    onCopy={onCopy}
-                    onMove={onMove}
-                  />
-                </div>
-              </ItemDragSource>
+              <div key={item.id} className="relative">
+                <PinnedItemSortDropTarget
+                  isFrontTarget
+                  itemModel="dataset"
+                  pinIndex={item.collection_position}
+                  enableDropTargetBackground={false}
+                />
+                <ItemDragSource item={item} collection={collection}>
+                  <div>
+                    <PinnedItemCard
+                      item={item}
+                      collection={collection}
+                      onCopy={onCopy}
+                      onMove={onMove}
+                    />
+                  </div>
+                </ItemDragSource>
+                <PinnedItemSortDropTarget
+                  isBackTarget
+                  itemModel="dataset"
+                  pinIndex={item.collection_position}
+                  enableDropTargetBackground={false}
+                />
+              </div>
             ))}
           </Grid>
         </div>

frontend/src/metabase/collections/components/PinnedItemOverview/PinnedItemOverview.unit.spec.js

 import React from "react";
-import { render } from "@testing-library/react";
+import { renderWithProviders, screen } from "__support__/ui";
 
 import PinnedItemOverview from "./PinnedItemOverview";
 
@@ -13,26 +13,38 @@ const defaultCollection = {
   archived: false,
 };
 
-const defaultItem = {
+const dashboardItem1 = {
   id: 1,
   model: "dashboard",
-  collection_position: 1,
+  collection_position: 2,
   name: "Dashboard Foo",
-  description: "description foo foo foo",
+  description: "description foo",
   getIcon: () => ({ name: "dashboard" }),
   getUrl: () => "/dashboard/1",
   setArchived: jest.fn(),
   setPinned: jest.fn(),
 };
 
+const dashboardItem2 = {
+  id: 2,
+  model: "dashboard",
+  collection_position: 1,
+  name: "Dashboard Bar",
+  description: "description foo",
+  getIcon: () => ({ name: "dashboard" }),
+  getUrl: () => "/dashboard/2",
+  setArchived: jest.fn(),
+  setPinned: jest.fn(),
+};
+
 function setup({ items, collection } = {}) {
-  items = items || [defaultItem];
+  items = items || [dashboardItem1, dashboardItem2];
   collection = collection || defaultCollection;
 
   mockOnCopy.mockReset();
   mockOnMove.mockReset();
 
-  return render(
+  return renderWithProviders(
     <PinnedItemOverview
       items={items}
       collection={collection}
@@ -41,6 +53,9 @@ function setup({ items, collection } = {}) {
       onMove={mockOnMove}
       onDrop={jest.fn()}
     />,
+    {
+      withDND: true,
+    },
   );
 }
 
@@ -51,4 +66,17 @@ describe("PinnedItemOverview", () => {
       "Save your questions, dashboards, and models in collections — and pin them to feature them at the top.",
     );
   });
+
+  it("should render items", () => {
+    setup();
+    expect(screen.getByText(dashboardItem1.name)).toBeInTheDocument();
+    expect(screen.getByText(dashboardItem2.name)).toBeInTheDocument();
+  });
+
+  it("should render items sorted by collection_position", () => {
+    setup();
+    const names = screen.queryAllByText(/Dashboard (Foo|Bar)/);
+    expect(names[0].textContent).toContain(dashboardItem2.name);
+    expect(names[1].textContent).toContain(dashboardItem1.name);
+  });
 });

frontend/src/metabase/collections/components/PinnedItemSortDropTarget/PinnedItemSortDropTarget.styled.tsx

+import styled from "styled-components";
+
+import PinnedItemSortDropTarget from "metabase/containers/dnd/PinnedItemSortDropTarget";
+import { GAP_REM } from "metabase/collections/components/PinnedItemOverview/PinnedItemOverview.styled";
+import { color } from "metabase/lib/colors";
+
+export type PinDropTargetProps = {
+  isBackTarget?: boolean;
+  isFrontTarget?: boolean;
+  itemModel: string;
+  pinIndex?: number | null;
+  enableDropTargetBackground?: boolean;
+};
+
+export type PinDropTargetRenderArgs = PinDropTargetProps & {
+  hovered: boolean;
+  highlighted: boolean;
+};
+
+export const StyledPinDropTarget = styled<PinDropTargetProps>(
+  PinnedItemSortDropTarget,
+)`
+  position: absolute !important;
+  top: 0;
+  bottom: 0;
+  left: -${(GAP_REM * 5) / 8}rem;
+  right: -${(GAP_REM * 5) / 8}rem;
+  pointer-events: none;
+  background-color: transparent;
+
+  * {
+    pointer-events: all;
+  }
+`;
+
+export const PinDropTargetIndicator = styled.div<PinDropTargetRenderArgs>`
+  z-index: 1;
+  position: absolute;
+  top: 0;
+  bottom: 0;
+  left: 0;
+  right: 0;
+  border-left: ${props =>
+    props.isFrontTarget &&
+    `4px solid ${props.hovered ? color("brand") : color("bg-medium")}`};}
+  border-right: ${props =>
+    props.isBackTarget &&
+    `4px solid ${props.hovered ? color("brand") : color("bg-medium")}`};}
+  display: ${props => !(props.hovered || props.highlighted) && "none"};
+`;

frontend/src/metabase/collections/components/PinnedItemSortDropTarget/PinnedItemSortDropTarget.tsx

+import React from "react";
+import {
+  StyledPinDropTarget,
+  PinDropTargetIndicator,
+  PinDropTargetProps,
+  PinDropTargetRenderArgs,
+} from "./PinnedItemSortDropTarget.styled";
+
+function PinnedItemSortDropTarget(props: PinDropTargetProps) {
+  return (
+    <StyledPinDropTarget {...props}>
+      {(args: PinDropTargetRenderArgs) => <PinDropTargetIndicator {...args} />}
+    </StyledPinDropTarget>
+  );
+}
+
+export default PinnedItemSortDropTarget;

frontend/src/metabase/collections/components/PinnedItemSortDropTarget/index.ts

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

frontend/src/metabase/collections/containers/CollectionContent.jsx

@@ -150,17 +150,14 @@ function CollectionContent({
                 )}
                 handleToggleMobileSidebar={handleToggleMobileSidebar}
               />
-              {!loadingPinnedItems && (
-                <PinnedItemOverview
-                  items={pinnedItems}
-                  collection={collection}
-                  metadata={metadata}
-                  onMove={handleMove}
-                  onCopy={handleCopy}
-                  onToggleSelected={toggleItem}
-                  onDrop={clear}
-                />
-              )}
+              <PinnedItemOverview
+                items={pinnedItems}
+                collection={collection}
+                metadata={metadata}
+                onMove={handleMove}
+                onCopy={handleCopy}
+                onToggleSelected={toggleItem}
+              />
               <Search.ListLoader
                 query={unpinnedQuery}
                 loadingAndErrorWrapper={false}

frontend/src/metabase/containers/dnd/DropArea.jsx

@@ -55,13 +55,16 @@ export default class DropArea extends React.Component {
       children,
       className,
       style,
+      enableDropTargetBackground = true,
       ...props
     } = this.props;
     return this.state.show
       ? connectDropTarget(
           <div className={cx("relative", className)} style={style}>
             {typeof children === "function" ? children(props) : children}
-            <DropTargetBackgroundAndBorder {...props} />
+            {enableDropTargetBackground && (
+              <DropTargetBackgroundAndBorder {...props} />
+            )}
           </div>,
         )
       : null;

frontend/src/metabase/containers/dnd/ItemDragSource.jsx

@@ -43,7 +43,7 @@ import { dragTypeForItem } from ".";
 
           onDrop && onDrop();
         } catch (e) {
-          alert("There was a problem moving these items: " + e);
+          console.error("There was a problem moving these items: " + e);
         }
       }
     },

frontend/src/metabase/containers/dnd/ItemsDragLayer.jsx

@@ -43,6 +43,7 @@ export default class ItemsDragLayer extends React.Component {
           transform: `translate(${x}px, ${y}px)`,
           pointerEvents: "none",
           opacity: 0.65,
+          zIndex: 1,
         }}
       >
         <DraggedItems

frontend/src/metabase/containers/dnd/PinDropTarget.jsx

 import { DropTarget } from "react-dnd";
+import PropTypes from "prop-types";
+
+import { isItemPinned } from "metabase/collections/utils";
 
 import DropArea from "./DropArea";
 import { PinnableDragTypes } from ".";
@@ -13,9 +16,16 @@ const PinDropTarget = DropTarget(
     },
     canDrop(props, monitor) {
       const { item } = monitor.getItem();
+      const { variant } = props;
       // NOTE: not necessary to check collection permission here since we
       // enforce it when beginning to drag and item within the same collection
-      return props.pinIndex !== item.collection_position;
+      if (variant === "pin") {
+        return !isItemPinned(item);
+      } else if (variant === "unpin") {
+        return isItemPinned(item);
+      }
+
+      return false;
     },
   },
   (connect, monitor) => ({
@@ -25,4 +35,8 @@ const PinDropTarget = DropTarget(
   }),
 )(DropArea);
 
+PinDropTarget.propTypes = {
+  variant: PropTypes.oneOf(["pin", "unpin"]).isRequired,
+};
+
 export default PinDropTarget;

frontend/src/metabase/containers/dnd/PinnedItemSortDropTarget.jsx

+import { DropTarget } from "react-dnd";
+import PropTypes from "prop-types";
+
+import { isItemPinned } from "metabase/collections/utils";
+
+import DropArea from "./DropArea";
+import { PinnableDragTypes } from ".";
+
+const PinnedItemSortDropTarget = DropTarget(
+  PinnableDragTypes,
+  {
+    drop(props, monitor, component) {
+      if (!props.noDrop) {
+        return { pinIndex: props.pinIndex };
+      }
+    },
+    canDrop(props, monitor) {
+      const { item } = monitor.getItem();
+      const { isFrontTarget, isBackTarget, itemModel, pinIndex } = props;
+
+      // NOTE: not necessary to check collection permission here since we
+      // enforce it when beginning to drag and item within the same collection
+      if (!isItemPinned(item)) {
+        return false;
+      }
+
+      if (itemModel != null && item.model !== itemModel) {
+        return false;
+      }
+
+      if (isFrontTarget) {
+        const isInFrontOfItem = pinIndex < item.collection_position;
+        return isInFrontOfItem;
+      } else if (isBackTarget) {
+        const isBehindItem = pinIndex > item.collection_position;
+        return isBehindItem;
+      }
+
+      return false;
+    },
+  },
+  (connect, monitor) => ({
+    highlighted: monitor.canDrop(),
+    hovered: monitor.isOver() && monitor.canDrop(),
+    connectDropTarget: connect.dropTarget(),
+  }),
+)(DropArea);
+
+PinnedItemSortDropTarget.propTypes = {
+  isFrontTarget: PropTypes.bool,
+  isBackTarget: PropTypes.bool,
+  itemModel: PropTypes.string,
+  pinIndex: PropTypes.number,
+};
+
+export default PinnedItemSortDropTarget;

frontend/src/metabase/containers/dnd/index.js

@@ -15,6 +15,7 @@ export const PinnableDragTypes = [
   DragTypes.QUESTION,
   DragTypes.DASHBOARD,
   DragTypes.PULSE,
+  DragTypes.DATASET,
 ];
 
 export const MoveableDragTypes = [