feat: [SC-43819] draggable table rows #987

bsholmes · 2023-12-15T16:58:07Z

Add drag events to GridTable rows in order to enable drag & drop reordering

https://app.shortcut.com/homebound-team/story/43819/implement-drag-drop-reordering-for-the-configure-options-table

shortcut-integration · 2023-12-15T16:58:10Z

This pull request has been linked to Shortcut Story #43819: Implement Drag & Drop reordering for the configure options table.

stephenh · 2023-12-15T17:37:04Z

src/components/Table/GridTable.stories.tsx

+/**
+ * Shows how drag & drop reordering can be implemented with GridTable drag events
+ */
+export function DraggableRows() {


Praise: I popped this open in storybook and it looks slick!

Seems like the only thing this doesn't do is scroll the page/table when I get to the top and bottom, but otherwise it seems pretty functionally complete? Very nice! Pretty great that it's just native events too, and not bringing in a new library. Kinda assumed we'd just need that, but this looks good!

It was scrolling for me when dragging to top/bottom, thought there seems to be a small window of where you need to hold the row for it to work 🤔

praise: Looks great @bsholmes

Yeah the scrolling behavior isn't ideal, and is relying on the dataTransfer.effectAllowed = "move" stuff so isn't tweakable. We may need to implement our own scrolling behavior, at least on mobile.

stephenh · 2023-12-15T17:37:36Z

src/components/Table/GridTable.stories.tsx

+    spacer: " ",
+    clientSideSort: false,
+  };
+  const spacerRow: GridDataRow<OrderedRow<Data>> = {


Thought: I think this spacer row is a good idea; afaict it like follows the mouse around to show where the being-dragged row will end up going?

stephenh · 2023-12-15T17:40:20Z

src/components/Table/GridTable.stories.tsx

+      onRowDragEnd={{ data: onDragEnd, spacer: onDragEnd }}
+      onRowDrop={{ data: onDrop, spacer: onDrop }}
+      onRowDragEnter={{ data: onDragEnter, spacer: onDragEnter }}
+      onRowDragOver={{ data: onDragOver, spacer: onDragOver }}


Thought: I think this makes a ton of sense for prototyping/spiking; wondering if maybe before shipping the final API might be more short & sweet, like something like:

onRowMove((row, oldIndex, newIndex) => ...)

And have all of the low-level drag math, and ideally like management of that spacer row (being injected/moving around within the page's rows), all managed internally for the page... 🤔

We could also add a few props to our GridStyle, like dragSpacerRow: { ...css stuff... } to hopefully both have our out-of-the-box styles support it already, but also pages could tweak it if/as needed.

bdow · 2023-12-15T17:47:39Z

src/components/Table/components/Row.tsx

@@ -372,4 +379,6 @@ export type GridDataRow<R extends Kinded> = {
  selectable?: false;
  /** Whether this row should infer its selected state based on its children's selected state */
  inferSelectedState?: false;
+  /** Whether this row is draggable, usually to allow drag & drop reordering of rows */
+  draggable?: true;


nit: Probably worth making this a boolean otherwise TS will sometimes require us to do draggable: true as const. If we allow it to be a boolean value, then we don't need the as const suffix.

bdow · 2023-12-15T17:50:19Z

src/components/Table/components/Row.tsx

@@ -113,7 +120,7 @@ function RowImpl<R extends Kinded, S>(props: RowProps<R>): ReactElement {
  let expandColumnHidden = false;

  return (
-    <RowTag css={rowCss} {...others} data-gridrow {...getCount(row.id)}>
+    <RowTag css={rowCss} {...others} data-gridrow {...getCount(row.id)} draggable={row.draggable}>


thought/suggestion: I think it'd be great to make a specific "drag handle" and test out what that looks like as well.
If we have a row with many other interactive elements inside of it we could have some potentially conflicting interactions (i dunno if that'd be true, but I'd rather avoid it).

Yeah I'm planning on adding that as well. Specifically on mobile I think it is necessary since dragging is the default behavior for scrolling through the table.

bdow

Nice spike! I agree with Stephen regarding simplifying the API so not every caller needs to implement all the onDragFoo properties.

bdow

Would also be great to test this out on our as="table" and as="virtual" variants.
When we use as="virtual" we are leveraging the React-Virtuoso library, which has an example of how to integrate with React Beautiful DnD: https://virtuoso.dev/react-beautiful-dnd/

stephenh · 2023-12-18T15:00:53Z

src/components/Table/GridTable.stories.tsx

+    order: idx + 1,
+    data: { name: "" + (idx + 1), value: idx + 1 },
+    draggable: true,
+  }));


@bsholmes I think one of the things that @bdow noticed in the demo is that the data-render HTML attributes of the row elements were going way up; we use those to make sure React is not completely re-rendering every table row on every render.

Definitely fine for the POC, but I think our goal for this PR, probably even a requirement? for v1, is that as the rows are dragged up/down, React doesn't re-render them at all, and instead they're just swapped in the DOM or what not.

I'm kinda surprised, because at first I thought "oh this rowArray is re-created on each render/something was going to be a hopefully easy fix for this, but your story is only using this for initializing of [rows, setRows], and then afaict everything after that is keeping the same "stable row identities" and just swapping them around in the list...

Which I was really hoping would be all we'd need to get for the "don't re-render each row" stuff to work, but it sounds like it's not...

Is that something you've observed/are thinking about/have any ideas yet as to why data-render isn't stable as we drag?

Hmm, that is a good call-out. I'm not sure why that would be happening here but I will look into it before I get a final PR ready.

…css spacer

stephenh · 2024-01-09T01:00:35Z

src/components/Table/GridTable.tsx

+    }
+
+    evt.dataTransfer.effectAllowed = "move";
+    evt.dataTransfer.setData("text/plain", JSON.stringify({ row }));


Thought: The row might be a little big, although maybe it doesn't matter? I'd be tempted to just do like setdata("text/plain", row.id) because the row ids should be unique.

stephenh

The code looks really neat/succinct; I think only actually issue is useCallback-ing the functions to get GridTable perf okay (keep all the rows memoized), by either just-use-useCallback or by moving the function declarations someplace already-stable like RowStates (my preference but likely because I like mobx :-).

stephenh · 2024-01-09T01:01:57Z

src/components/Table/components/Row.tsx

@@ -81,6 +94,8 @@ function RowImpl<R extends Kinded, S>(props: RowProps<R>): ReactElement {
  const levelIndent = style.levels && style.levels[level]?.rowIndent;

  const rowCss = {
+    // ...Css.add("transition", "padding-top 0.5s ease-in-out").$,


Nit: can remove

stephenh · 2024-01-09T01:08:38Z

src/components/Table/utils/RowState.ts

@@ -59,6 +67,7 @@ export class RowState<R extends Kinded> {
        _row: false,
        _data: observable.ref,
        isCalculatingDirectMatch: false,
+        isDraggedOver: observable.ref, // allows the table to re-render only this row when the dragged over state changes


Question: Given that isDraggedOver is a primitive, I think this is unnecessary? Usually the observable.ref short-circuits mobx from doing a deep compare, which is how primitives basically work by default.

stephenh · 2024-01-09T01:09:49Z

src/components/Table/GridTable.tsx

@@ -274,6 +290,111 @@ export function GridTable<R extends Kinded, X extends Only<GridTableXss, X> = an
  const expandedColumnIds: string[] = useComputed(() => tableState.expandedColumnIds, [tableState]);
  const columnSizes = useSetupColumnSizes(style, columns, resizeTarget ?? resizeRef, expandedColumnIds);

+  function isCursorBelowMidpoint(target: HTMLElement, clientY: number) {


Minor: Maybe move to the utils file given afaict the function doesn't need to capture any state.

stephenh · 2024-01-09T01:16:05Z

src/components/Table/GridTable.tsx

+    });
+  }
+
+  function onDragStart(row: GridDataRow<R>, evt: DragEventType) {


Suggestion: I think useCallback-ing all of these functions will be necessary to keep the all rows from re-rendering on each GridTable render. As-is the Row props have different values/new functions on each GridTable render.

...it might be "too cute" but fwiw that RowStates / TableState classes are effectively already cached/1:1 with each GridTable, so if you did something like TableState.onDragStart (all of these become methods on TableState or RowStates), and then inside of of Row you could do onDragStart => tableState.onDragStart(row, evt).

I guess you'd need the setDraggedRow setter, but you could do something like a TableState.draggedRow field, that just gets set... (i.e. skip the useState + useRef pair, and just use a field on the mobx TableState to achieve basically the same thing...

I added a test to GridTable.test.js to look at row rerenders for draggable rows and I believe they already only rerender on data change, just like the non-draggable rows, with the exception that they need to rerender when I'm setting css for the spacer and when they get reordered.

stephenh · 2024-01-09T01:25:38Z

src/components/Table/components/Row.tsx

+      ref={ref}
+    >
+      {row.draggable && (
+        <div


Thought: I kinda think we need this div to go inside one of the columns, otherwise we're throwing off the column alignment between the header row (2 divs/columns) & the table rows (3 divs/columns)...

Maybe fine to ship for now/for our usage?

We could potentially give users a <DragHandle /> component that we just tell them "put this into your 1st column" ... or a dragColumn helper like in our columns.tsx in beam, there is a dateColumn, actionColumn, etc... That would be a place to put this code that doesn't throw off the column alignment...

Oh, yeah, the selectColumn and collapseColumn in particular handle being dropped into any table/without knowing the table's kinds...

Good call, I added a drag handle column as you suggested. The only thing that makes this a little odd is that setting the draggable flag and providing a drop callback won't really be usable unless that column is also provided.

...won't really be usable unless that column is also provided.

True...if you wanted, you could do something like "if (any row has draggable || drop callback set) && (!columns has a drag column) throw error("please add a drag column").

That sort of polished UX/DX isn't really common/expected, but pretty neat/appreciated when we do have it. :-)

chr1sjf0x

Looks pretty good to me overall. I'll defer to @stephenh on the final 👍 given his suggestions.

chr1sjf0x · 2024-01-09T21:31:44Z

src/components/Table/GridTable.stories.tsx

+  );
+}
+
+export const DraggableWithInputColumns = newStory(


curious @bsholmes - For these last 2 columns, the dragged item does not remain in the position it is dropped. Is that expected? Am I doing something wrong like dropping things where they are not allowed to be?

I neglected to implement the drop event handler in those stories, should be there now

## [2.333.0](v2.332.1...v2.333.0) (2024-01-10) ### Features * [SC-43819] draggable table rows ([#987](#987)) ([16b883b](16b883b))

homebound-team-bot · 2024-01-10T19:53:12Z

🎉 This PR is included in version 2.333.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

bsholmes added 3 commits December 14, 2023 14:30

rough draft drag & drop

e3d89d9

cleanup, make spacer droppable

797ab1f

Merge branch 'main' into sc-43819-draggable-table-rows

19f0a48

bsholmes requested review from stephenh, bdow, ksk5280 and chr1sjf0x as code owners December 15, 2023 16:58

linting

493afc1

bsholmes changed the title ~~[SC-43819] draggable table rows~~ feat: [SC-43819] draggable table rows Dec 15, 2023

stephenh reviewed Dec 15, 2023

View reviewed changes

bdow reviewed Dec 15, 2023

View reviewed changes

stephenh reviewed Dec 18, 2023

View reviewed changes

bsholmes added 12 commits December 21, 2023 10:16

WIP event refactor

8f5e404

refactor to a single event callback, using index as order, and using …

c64504a

…css spacer

Merge branch 'main' into sc-43819-draggable-table-rows

5ce7af3

remove unused

b472550

more cleanup and linting

6b24237

more cleanup

b3fdc89

comment

cd5d4cc

fix memoized render when no callback is defined

ff644f3

lint

008cc16

fix drag events always causing rerenders

35763c1

drag handle, index offset

3b2ac9c

test for draggable re-renders

7a8fbb4

bsholmes added 3 commits January 8, 2024 15:11

handle children gracefully while dragging

5048376

cleanup

abc6e72

Merge branch 'main' into sc-43819-draggable-table-rows

567637b

stephenh reviewed Jan 9, 2024

View reviewed changes

chr1sjf0x reviewed Jan 9, 2024

View reviewed changes

bsholmes added 5 commits January 9, 2024 17:13

draghandle column component, child reordering in story, some cleanup

fd3b607

lint

98f18d5

export

cfc3672

memoize test for row rerender

83efc86

lint

2f42520

stephenh approved these changes Jan 10, 2024

View reviewed changes

fix for Firefox

92c5498

bsholmes merged commit 16b883b into main Jan 10, 2024
6 checks passed

bsholmes deleted the sc-43819-draggable-table-rows branch January 10, 2024 19:50

homebound-team-bot pushed a commit that referenced this pull request Jan 10, 2024

chore(release): 2.333.0 [skip ci]

e57d034

## [2.333.0](v2.332.1...v2.333.0) (2024-01-10) ### Features * [SC-43819] draggable table rows ([#987](#987)) ([16b883b](16b883b))

homebound-team-bot added the released label Jan 10, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: [SC-43819] draggable table rows #987

feat: [SC-43819] draggable table rows #987

bsholmes commented Dec 15, 2023 •

edited

Loading

shortcut-integration bot commented Dec 15, 2023

stephenh Dec 15, 2023

stephenh Dec 15, 2023

chr1sjf0x Dec 15, 2023

bsholmes Dec 15, 2023

stephenh Dec 15, 2023

stephenh Dec 15, 2023

bdow Dec 15, 2023

bdow Dec 15, 2023

bsholmes Dec 15, 2023

bdow left a comment

bdow left a comment

stephenh Dec 18, 2023

bsholmes Dec 21, 2023

stephenh Jan 9, 2024

stephenh left a comment

stephenh Jan 9, 2024

stephenh Jan 9, 2024

stephenh Jan 9, 2024

stephenh Jan 9, 2024

bsholmes Jan 10, 2024

stephenh Jan 9, 2024

bsholmes Jan 10, 2024

stephenh Jan 10, 2024

chr1sjf0x left a comment

chr1sjf0x Jan 9, 2024

bsholmes Jan 10, 2024 •

edited

Loading

homebound-team-bot commented Jan 10, 2024

feat: [SC-43819] draggable table rows #987

feat: [SC-43819] draggable table rows #987

Conversation

bsholmes commented Dec 15, 2023 • edited Loading

shortcut-integration bot commented Dec 15, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bdow left a comment

Choose a reason for hiding this comment

bdow left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephenh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chr1sjf0x left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bsholmes Jan 10, 2024 • edited Loading

Choose a reason for hiding this comment

homebound-team-bot commented Jan 10, 2024

bsholmes commented Dec 15, 2023 •

edited

Loading

bsholmes Jan 10, 2024 •

edited

Loading