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

bsholmes · 2024-01-12T19:05:09Z

Card style rows don't work with padding as spacing, and we can't use margin and still get drop events. So we need a containing element for padding and drag & drop events.

shortcut-integration · 2024-01-12T19:05:12Z

This pull request has been linked to Shortcut Story #43820: Configure Options Table Reordering - Options within groups.

bsholmes · 2024-01-12T19:07:15Z

src/components/Table/components/Row.tsx

-      {...getCount(row.id)}
+    // we need to use padding for the drag & drop spacing or else drop events don't fire
+    // and we need a containing element for padding to work correctly with the card style row
+    <div


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.

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.

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.

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.

conditional it is

@stephenh should only affect draggable rows now

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

@bsholmes nice, thank you!

bdow

I noticed some potential re-rendering perf issues that might come up. I've noted it in the loom here:
https://www.loom.com/share/4834d010e0c34bbcbb7f011deb27c3e6

Another question I had was if we ever looked at inserting a simple dom element into the position that is being dragged over vs adjusting padding on an existing element? I can see how the virtualized tables may not like us adding/removing dom elements on the fly, but would have been cool to check it out. We did something similar to that in the DnDGrid. All the dragging and dropping logic happens outside of React state there, which allows for zero rerenders during the drag and drop process.
React-Virtuoso (which we use in GridTable) provides a really good example of how they worked with React Beautiful DnD: https://virtuoso.dev/react-beautiful-dnd/. It looks like a super simple implementation.

bdow · 2024-01-12T21:32:52Z

src/components/Table/components/Row.tsx

+    ...(rs.isDraggedOver === DraggedOver.Above && Css.add("paddingTop", "35px").$),
+    ...(rs.isDraggedOver === DraggedOver.Below && Css.add("paddingBottom", "35px").$),


nit: Truss has a lot of great shorthand ways or writing styles that hopefully make this sort of thing less verbose and easier. You could easily do these padding styles via: Css.ptPx(35).$.

bdow · 2024-01-12T21:34:42Z

src/components/Table/components/Row.tsx

@@ -93,8 +92,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 rowCss = {
+  const containerCss = {
    ...Css.add("transition", "padding 0.5s ease-in-out").$,


nit/question: This timing feels a bit slow. Have you been able to work with Design to define the proper behaviors/timings for the drag and drop?

Nope, sure haven't, but I made it a bit snappier

bdow · 2024-01-12T21:41:26Z

src/components/Table/GridTable.stories.tsx

+  return (
+    <GridTable
+      columns={[dragColumn, nameColumn, actionColumn]}
+      onRowDrop={(draggedRow, droppedRow, indexOffset) => {


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.

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.

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.

## [2.334.0](v2.333.1...v2.334.0) (2024-01-16) ### Features * Drag & Drop fast follow for card style rows ([#991](#991)) ([70040be](70040be))

homebound-team-bot · 2024-01-16T19:47:38Z

🎉 This PR is included in version 2.334.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

row container div for card style and

dfc1be0

bsholmes requested review from stephenh, bdow and chr1sjf0x January 12, 2024 19:05

bsholmes requested a review from ksk5280 as a code owner January 12, 2024 19:05

bsholmes commented Jan 12, 2024

View reviewed changes

bsholmes added 5 commits January 12, 2024 11:47

lint

f4bc9de

conditionally use drag container if row is draggable

bdda5b3

lint

bce35af

function call

420f710

lint

13deb4d

bdow approved these changes Jan 12, 2024

View reviewed changes

bsholmes added 2 commits January 12, 2024 13:55

cleaner truss, snappier transition

b2cc979

Merge branch 'main' into sc-43820-drag-and-drop-fast-follow

c140013

stephenh approved these changes Jan 12, 2024

View reviewed changes

bsholmes added 2 commits January 16, 2024 10:58

fix 'jiggling' issues and debounce dragOver

a8831c6

lint

26f73f8

bsholmes merged commit 70040be into main Jan 16, 2024
6 checks passed

bsholmes deleted the sc-43820-drag-and-drop-fast-follow branch January 16, 2024 19:44

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

chore(release): 2.334.0 [skip ci]

f9e1954

## [2.334.0](v2.333.1...v2.334.0) (2024-01-16) ### Features * Drag & Drop fast follow for card style rows ([#991](#991)) ([70040be](70040be))

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

bsholmes commented Jan 12, 2024

shortcut-integration bot commented Jan 12, 2024

bsholmes Jan 12, 2024

stephenh Jan 12, 2024

bsholmes Jan 12, 2024

stephenh Jan 12, 2024

bsholmes Jan 12, 2024

bsholmes Jan 12, 2024

bdow Jan 12, 2024

stephenh Jan 12, 2024

bdow left a comment

bdow Jan 12, 2024

bdow Jan 12, 2024

bsholmes Jan 12, 2024

bdow Jan 12, 2024

bsholmes Jan 12, 2024

bdow Jan 12, 2024

homebound-team-bot commented Jan 16, 2024

		...(rs.isDraggedOver === DraggedOver.Above && Css.add("paddingTop", "35px").$),
		...(rs.isDraggedOver === DraggedOver.Below && Css.add("paddingBottom", "35px").$),

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

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

Conversation

bsholmes commented Jan 12, 2024

shortcut-integration bot commented Jan 12, 2024

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

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

homebound-team-bot commented Jan 16, 2024