Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Drag & Drop fast follow for card style rows #991

Merged
merged 10 commits into from
Jan 16, 2024
Merged

feat: Drag & Drop fast follow for card style rows #991

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
dfc1be0
row container div for card style and
bsholmes Jan 12, 2024
f4bc9de
lint
bsholmes Jan 12, 2024
bdda5b3
conditionally use drag container if row is draggable
bsholmes Jan 12, 2024
bce35af
lint
bsholmes Jan 12, 2024
420f710
function call
bsholmes Jan 12, 2024
13deb4d
lint
bsholmes Jan 12, 2024
b2cc979
cleaner truss, snappier transition
bsholmes Jan 12, 2024
c140013
Merge branch 'main' into sc-43820-drag-and-drop-fast-follow
bsholmes Jan 12, 2024
a8831c6
fix 'jiggling' issues and debounce dragOver
bsholmes Jan 16, 2024
26f73f8
lint
bsholmes Jan 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 45 additions & 0 deletions src/components/Table/GridTable.stories.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2020,3 +2020,48 @@ export function DraggableNestedRows() {
/>
);
}

export function DraggableCardRows() {
const dragColumn = dragHandleColumn<Row>({});
const nameColumn: GridColumn<Row> = {
header: "Name",
data: ({ name }) => ({ content: <div>{name}</div>, sortValue: name }),
};

const actionColumn: GridColumn<Row> = {
header: "Action",
data: () => <div>Actions</div>,
clientSideSort: false,
};

let rowArray: GridDataRow<Row>[] = new Array(26).fill(0);
rowArray = rowArray.map((elem, idx) => ({
kind: "data",
id: "" + (idx + 1),
order: idx + 1,
data: { name: "" + (idx + 1), value: idx + 1 },
draggable: true,
}));

const [rows, setRows] = useState<GridDataRow<Row>[]>([simpleHeader, ...rowArray]);

// also works with as="table" and as="virtual"
return (
<GridTable
columns={[dragColumn, nameColumn, actionColumn]}
onRowDrop={(draggedRow, droppedRow, indexOffset) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question: I'm a little late to the game here, but wouldn't draggedRow and droppedRow be the same row?
It is probably too late to go back and update the implementation at this point, but just spit-balling here; Could we have gotten away with a callback like:

onRowDrop: (row: GridDataRow, newIndex: number) => void

This would give the user the information that a certain row was moved to newIndex.

Or many even:

onRowDrop: (newRows: GridDataRow[]) => void

If the developer only ever needs to just update their row property, this might make their implementations simpler.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

draggedRow would be the row that was dragged initially, droppedRow would be the row we just dropped the draggedRow onto. We could potentially simplify this callback if we only want it to support reordering, but potentially it could support other kinds of drag & drop behavior and in those cases we want the handler to have info about both rows.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, yes. That is a bit confusing of a name then. I would've assume the row being "dropped" is the row I was dragging. Maybe targetRow is a better name?
Interesting point about other drag and drop behaviors. Are we being asked to support dropping into another row in order to make the draggedRow a child of the target/droppedRow?
I'd be tempted to support multiple callbacks for that... like onReorder for the current use case, and .... I dunno what else? Would we want to allow rows to drop into each other to nest them or something? I'd be tempted to solution only the problem in front of us instead of envision future scenarios.
I'm fine with leaving it as is for now.

const tempRows = [...rows];
// remove dragged row
const draggedRowIndex = tempRows.findIndex((r) => r.id === draggedRow.id);
const reorderRow = tempRows.splice(draggedRowIndex, 1)[0];

const droppedRowIndex = tempRows.findIndex((r) => r.id === droppedRow.id);

// insert it at the dropped row index
setRows([...insertAtIndex(tempRows, reorderRow, droppedRowIndex + indexOffset)]);
}}
rows={[...rows]}
style={cardStyle}
/>
);
}
9 changes: 7 additions & 2 deletions src/components/Table/GridTable.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -352,7 +352,11 @@ export function GridTable<R extends Kinded, X extends Only<GridTableXss, X> = an
evt.preventDefault();

// set flags for css spacer
recursiveSetDraggedOver(rows, DraggedOver.None);
// don't set none for the row we are entering
recursiveSetDraggedOver(
rows.filter((r) => r.id !== row.id),
DraggedOver.None,
);

if (draggedRowRef.current) {
if (draggedRowRef.current.id === row.id) {
Expand All @@ -373,7 +377,8 @@ export function GridTable<R extends Kinded, X extends Only<GridTableXss, X> = an
evt.preventDefault();

if (draggedRowRef.current) {
if (draggedRowRef.current.id === row.id) {
if (draggedRowRef.current.id === row.id || !evt.currentTarget) {
tableState.maybeSetRowDraggedOver(row.id, DraggedOver.None, draggedRowRef.current);
return;
}

Expand Down
76 changes: 40 additions & 36 deletions src/components/Table/components/Row.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { observer } from "mobx-react";
import { ReactElement, useContext, useRef } from "react";
import { ReactElement, useContext, useRef, useCallback } from "react";
import {
defaultRenderFn,
headerRenderFn,
Expand Down Expand Up @@ -31,7 +31,7 @@ import {
import { Css, Palette } from "src/Css";
import { AnyObject } from "src/types";
import { isFunction } from "src/utils";
import { Icon } from "src";
import { useDebouncedCallback } from "use-debounce";

interface RowProps<R extends Kinded> {
as: RenderAs;
Expand Down Expand Up @@ -93,8 +93,13 @@ function RowImpl<R extends Kinded, S>(props: RowProps<R>): ReactElement {
const rowStyleCellCss = maybeApplyFunction(row as any, rowStyle?.cellCss);
const levelIndent = style.levels && style.levels[level]?.rowIndent;

const containerCss = {
...Css.add("transition", "padding 0.25s ease-in-out").$,
...(rs.isDraggedOver === DraggedOver.Above && Css.ptPx(25).$),
...(rs.isDraggedOver === DraggedOver.Below && Css.pbPx(25).$),
};

const rowCss = {
...Css.add("transition", "padding 0.5s ease-in-out").$,
...(!reservedRowKinds.includes(row.kind) && style.nonHeaderRowCss),
// Optionally include the row hover styles, by default they should be turned on.
...(showRowHoverColor && {
Expand All @@ -117,8 +122,6 @@ function RowImpl<R extends Kinded, S>(props: RowProps<R>): ReactElement {
[`:hover > .${revealOnRowHoverClass} > *`]: Css.visible.$,
},
...(isLastKeptRow && Css.addIn("&>*", style.keptLastRowCss).$),
...(rs.isDraggedOver === DraggedOver.Above && Css.add("paddingTop", "35px").$),
...(rs.isDraggedOver === DraggedOver.Below && Css.add("paddingBottom", "35px").$),
};

let currentColspan = 1;
Expand All @@ -131,36 +134,16 @@ function RowImpl<R extends Kinded, S>(props: RowProps<R>): ReactElement {
// used to render the whole row when dragging with the handle
const ref = useRef<HTMLTableRowElement>(null);

return (
<RowTag
css={rowCss}
{...others}
data-gridrow
{...getCount(row.id)}
// these events are necessary to get the dragged-over row for the drop event
// and spacer styling
onDrop={(evt) => onDrop?.(row, evt)}
onDragEnter={(evt) => onDragEnter?.(row, evt)}
onDragOver={(evt) => onDragOver?.(row, evt)}
ref={ref}
>
{/* {row.draggable && (
<div
draggable={row.draggable}
onDragStart={(evt) => {
// show the whole row being dragged when dragging with the handle
ref.current && evt.dataTransfer.setDragImage(ref.current, 0, 0);
return onDragStart?.(row, evt);
}}
onDragEnd={(evt) => onDragEnd?.(row, evt)}
onDrop={(evt) => onDrop?.(row, evt)}
onDragEnter={(evt) => onDragEnter?.(row, evt)}
onDragOver={(evt) => onDragOver?.(row, evt)}
css={Css.mh100.ma.$}
>
<Icon icon="drag" />
</div>
)} */}
// debounce drag over callback to avoid excessive re-renders
const dragOverCallback = useCallback(
(row: GridDataRow<R>, evt: React.DragEvent<HTMLElement>) => onDragOver?.(row, evt),
[onDragOver],
);
// when the event is not called, we still need to call preventDefault
const onDragOverDebounced = useDebouncedCallback(dragOverCallback, 100);

const RowContent = () => (
<RowTag css={rowCss} {...others} data-gridrow {...getCount(row.id)}>
{isKeptGroupRow ? (
<KeptGroupRow as={as} style={style} columnSizes={columnSizes} row={row} colSpan={columns.length} />
) : (
Expand Down Expand Up @@ -227,7 +210,7 @@ function RowImpl<R extends Kinded, S>(props: RowProps<R>): ReactElement {
onDragEnd,
onDrop,
onDragEnter,
onDragOver,
onDragOver: onDragOverDebounced,
});

// Only use the `numExpandedColumns` as the `colspan` when rendering the "Expandable Header"
Expand Down Expand Up @@ -372,6 +355,27 @@ function RowImpl<R extends Kinded, S>(props: RowProps<R>): ReactElement {
)}
</RowTag>
);

return row.draggable ? (
<div
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wrapping rows in this container seems to be the only sane way to make card style rows work with drag & drop. It doesn't seem to have broken anything wrt rows/tables, at least as far as Beam is concerned. But this is somewhat risky in terms of breaking implementations that rely on Beam. Let me know if this is likely to mess something up.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Huh, isn't RowTag basically already providing a wrapping div around the cells? It seems odd that we'd need two wrapping divs...

Ah, I get it, both dnd & cards were using padding, but when dragging we want the dnd padding to be "on top" / separate from the card padding?

I'm okay-ish with this as a short-term fix, but yeah let's try and only ad the extra div is dnd is actively being used, just to be extra sure we don't mess existing callers.

Afaiu does dnd use padding to provide the "spacer" effect? Kinda wondering if we could use a psuedo element for that (like the ::before thing), which I personally haven't done a ton of. Not sure how we'd listen to drop events on that, but maybe we could hook up the drop handlers to the root table div, and that would pick up the ::before element, maybe encode what got "dropped" using a data-something attribute on the psuedo element.

cc @bdow for thoughts.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The padding for the spacer goes inside the border of the card, so instead of looking like it's going above or below the row, it looks like it's going inside the card. I will look into the before/after pseudo-elements but I'm pretty sure they won't handle events properly. I could make the container conditional to draggable rows, but theoretically it should be able to work either way.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The padding for the spacer goes inside the border of the card, so instead of
looking like it's going above or below the row, it looks like it's going inside
the card.

Yeah, makes sense...

theoretically it should be able to work either way.

Yeah. Ngl I'm surprised it still works, but I get it, because we're like hand-calcing so much of the column widths/etc; even if it just comes down to DOM aesthetics of avoiding the extra div 🤷 dunno, seems simple enough to avoid/make conditional.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

conditional it is

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@stephenh should only affect draggable rows now

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was also thinking the pseudo elements would be the solution here instead of a wrapping element. I'm surprised they don't.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bsholmes nice, thank you!

css={containerCss}
// these events are necessary to get the dragged-over row for the drop event
// and spacer styling
onDrop={(evt) => onDrop?.(row, evt)}
onDragEnter={(evt) => onDragEnter?.(row, evt)}
onDragOver={(evt) => {
// when the event isn't called due to debounce, we still need to
// call preventDefault for the drop event to fire
evt.preventDefault();
onDragOverDebounced(row, evt);
}}
ref={ref}
>
{RowContent()}
</div>
) : (
<>{RowContent()}</>
);
}

/**
Expand Down
1 change: 1 addition & 0 deletions src/components/Table/utils/RowStates.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -203,6 +203,7 @@ export class RowStates<R extends Kinded> {
}

// this allows a single-row re-render
if (rs.isDraggedOver === draggedOver) return;
rs.isDraggedOver = draggedOver;
}
}
Expand Down
10 changes: 0 additions & 10 deletions src/components/Table/utils/columns.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -200,32 +200,22 @@ export function dragHandleColumn<T extends Kinded>(columnDef?: Partial<GridColum
id: "beamDragHandleColumn",
clientSideSort: false,
align: "center",
// Defining `w: 40px` to accommodate for the `16px` wide checkbox and `12px` of padding on either side.
w: "40px",
wrapAction: false,
isAction: true,
expandColumns: undefined,
// Select Column should not display the select toggle for `expandableHeader` or `totals` row kinds
expandableHeader: emptyCell,
totals: emptyCell,
// Use any of the user's per-row kind methods if they have them.
...columnDef,
};

// return newMethodMissingProxy(base, (key) => {
// return (data: any, { row, level }: { row: GridDataRow<any>; level: number }) => ({
// content: <CollapseToggle row={row} compact={level > 0} />,
// });
// }) as any;

return newMethodMissingProxy(base, (key) => {
return (data: any, { row, dragData }: { row: GridDataRow<T>; dragData: DragData<T> }) => {
if (!dragData) return;
const { rowRenderRef: ref, onDragStart, onDragEnd, onDrop, onDragEnter, onDragOver } = dragData;

return {
// how do we get the callbacks and the ref here?
// inject them into the row in the Row component?
content: row.draggable ? (
<div
draggable={row.draggable}
Expand Down
4 changes: 2 additions & 2 deletions src/components/Table/utils/utils.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -273,8 +273,8 @@ export function isCursorBelowMidpoint(target: HTMLElement, clientY: number) {
const style = window.getComputedStyle(target);
const rect = target.getBoundingClientRect();

const pt = parseInt(style.getPropertyValue("padding-top")) / 2;
const pb = parseInt(style.getPropertyValue("padding-bottom"));
const pt = parseFloat(style.getPropertyValue("padding-top"));
const pb = parseFloat(style.getPropertyValue("padding-bottom"));

return clientY > rect.top + pt + (rect.height - pb) / 2;
}
Expand Down
Loading