Use vertical line as drop target indicator for plots #1935

SkyLeite · 2022-06-21T13:58:42Z

This was a tricky one to do initially. My first idea was to simply change the DropTarget to a vertical line and change the dragged column to not be hidden, but that didn't work because the vertical line was still part of the table flow, meaning it would consume the entire width of a header.

My current approach passes the header element itself to DropTarget so it can render the header as well as the drop indicators.

Here's a video of how it looks: https://streamable.com/tgy6ew

TODO:

Fix not being able to drag a column to the first or last spot
Fix tests
Manually test in VSCode (I've been using Storybook for development)

Known issues:

Indicator looks thicker when the target is the first or last column
This wasn't intended during development, and it happens as a quirk of the indicator appearing under the headers when it's sandwiched between two of them, making it look thinner. Personally I think this can be repurposed into a good thing, as the line being thicker serves as a more clear indicator in this case where the user is less likely to notice the indicator at all.
Invalid DOM tree
Due to the way the indicator is placed in the DOM, it generates a DOM tree that fails react-dom's check as a div is not supposed to be a child of tr. I can't think of a way to get around this, as the indicator needs to be a child of the row to be appropriately placed.
Test should move a template plot from one type in another section of the same type and show two drop targets fails
I need some guidance for this one. I couldn't find the scenario it describes in storybook, and it also seems like it would no longer be relevant with the new Drop Target mechanism for plots, but I'm not entirely sure.

sroy3 · 2022-06-21T23:12:32Z

Test should move a template plot from one type in another section of the same type and show two drop targets fails
I need some guidance for this one. I couldn't find the scenario it describes in storybook, and it also seems like it would no longer be relevant with the new Drop Target mechanism for plots, but I'm not entirely sure.

You modified the behaviour of drag and drop for other plots (Trends and Data Series), it should stay the same. If the test still fails after, this is shown here by the last drag in the video:

Screen.Recording.2022-06-21.at.7.10.42.PM.mov

sroy3 · 2022-06-21T23:20:11Z

There is a little bug when dropping on an invalid target. The column will stay dark:

Screen.Recording.2022-06-21.at.7.15.48.PM.mov

Other than that, your implementation looks great. Great job! Getting around table behaviour is not an easy task.

Like I said previously, be careful not to change the behaviour of drag and drop elsewhere (checkpoints and template plots and column headers in the table) and the PR will be pretty solid after that.

SkyLeite · 2022-06-22T02:49:47Z

@sroy3 Thank you for the feedback and help! I managed to fix both issues, and even added back the border that was missing for the drop target of Trends and Data Series. As such I'm marking this as ready for review :)

rogermparent · 2022-06-22T04:15:06Z

nvalid DOM tree
Due to the way the indicator is placed in the DOM, it generates a DOM tree that fails react-dom's check as a div is not supposed to be a child of tr. I can't think of a way to get around this, as the indicator needs to be a child of the row to be appropriately placed.

Does using divs with display: table instead of trs get around this? If not, maybe we can re-create the styles with flexbox much like how react-table does so with the experiments table.

rogermparent

Great work so far! I've found a couple tiny nitpicks from a first read-over, but it seems like all the big issues were caught by Stephanie and you've already handled them! I'll be taking another look early tomorrow.

rogermparent · 2022-06-22T04:25:15Z

webview/src/shared/components/dragDrop/DragDropContainer.tsx

-      style={(id === draggedId && { display: 'none' }) || draggable.props.style}
+      style={
+        (id === draggedId && hideDragged && { display: 'none' }) ||
+        draggable.props.style
+      }


I see it was like this before this PR, but is the value of this style prop not just a weirdly written ternary? i.e. id === draggedId && hideDragged ? {display: 'none'} : draggable.props.style

Not sure in this particular case, but I think our linter favours this syntax over ternaries using ? and : sometimes.

Agreed. I changed it to a ternary and ESLint seemed to be fine with it in this case.

rogermparent · 2022-06-22T04:30:35Z

webview/src/plots/components/App.test.tsx

+    const target = screen.getByTestId('drop-target')
+
+    // eslint-disable-next-line testing-library/no-node-access
+    expect(target.nextElementSibling?.id).toStrictEqual(plots[0].id)


Could these just compare the elements' references? i.e. expect(target.nextElementSibling).toBe(plots[0])

Not super important, just makes things a tiny bit simpler. This understandably apes the existing implementation, so it's not like it comes from nowhere.

Good point. I agree with comparing by reference instead of id 👍

rogermparent · 2022-06-22T04:40:00Z

webview/src/plots/components/comparisonTable/DropTargetIndicator.tsx

+    className={styles.dropTargetIndicator}
+    data-testid="comparison-drop-target-indicator"
+    style={{
+      [direction.toLowerCase()]: -4


Using the transformed value of an enum directly as a string makes me a little uneasy, though it's not like avoiding doing so makes the code that much more elegant.

Something I've seen done in Elm on cases like this, since in Elm you can't derive a string from an enum, is a function that explicitly returns a string from an enum value. I thought about doing the same thing here, but it gave me "early optimization" vibes. Does team have any ideas or precedents in the code base for tackling this?

rogermparent · 2022-06-22T04:45:18Z

webview/src/plots/components/styles.module.scss

+  & > th {
+    display: block;
+    width: 100%;
+  }


Does this still apply to anything? It seems like the drop target was turned into a div in another part of the code, and changing a th's display seems like it would cause a lot more chaos if it were applying.

Good catch. It is applying because the current implementation re-renders the dragged cell as the drop target, which in this case is a th. The display: block is indeed unnecessary, but the width: 100% is there so the cell expands to fill the drop target.

sroy3

The drag and drop elsewhere is still not quite how it was before:

Screen.Recording.2022-06-22.at.9.06.41.AM.mov

There is a double border when the drop target is at the start:

Did you try changing the style of the dragged element like the mock? Was it too complicated?

sroy3 · 2022-06-22T12:55:30Z

webview/src/plots/components/App.test.tsx

-      plots[1].id,
-      plots[2].id
-    ])
+    const target = screen.getByTestId('drop-target')


I'm a little uneasy about changing a test for template plots drag and drop while the feature is meant to change only the comparison table.

Agreed. This should've been reverted in ba9c284 along with the other changes. It's done now.

…t items

SkyLeite · 2022-06-22T17:43:30Z

@sroy3

The drag and drop elsewhere is still not quite how it was before:

It should be fine as of e2e8caf. Do you think the approach of special casing based on the hideDragged prop makes sense? I feel like it works fine for now, but I'm concerned about introducing technical debt since it's not a very flexible approach. How does the team usually handle these scenarios?

There is a double border when the drop target is at the start:

Fixed in 31aebcf. Initially I thought that was a nice touch, as I find the indicator a little hard to see when it's not between two elements, but after seeing what it looks like in your clip (Safari, I presume) I agree that it's not the best way to solve this.

Did you try changing the style of the dragged element like the mock? Was it too complicated?

I did initially, but only came across solutions like setting and re-setting the element's style on dragStart, which doesn't even seem to work on modern browsers anymore. However, after reading your comment I decided to take another look and found documentation for setDragImage on MDN and that seems to work great! It's not very elegant because we need to append an element off-screen to be used as the ghost image, but it's what MDN recommends so I'd say it's good enough. I've implemented it in 0887cc0

SkyLeite · 2022-06-22T18:32:05Z

nvalid DOM tree
Due to the way the indicator is placed in the DOM, it generates a DOM tree that fails react-dom's check as a div is not supposed to be a child of tr. I can't think of a way to get around this, as the indicator needs to be a child of the row to be appropriately placed.

Does using divs with display: table instead of trs get around this? If not, maybe we can re-create the styles with flexbox much like how react-table does so with the experiments table.

I managed to fix this in 42f369d. The solution boils down to changing the wrapper element back to a th, and re-creating the element that's rendered by DropTarget as a div. It solves both issues (div cannot be a child of tr and th cannot be a child of div) without having to recreate the table layout.

I also tried using divs with display: table instead of trs, but that triggered the same issue in that divs cannot be children of thead. Following this rabbit hole would mean getting rid of the table altogether and creating a div soup all the way down, so I figured it was best to drop it in favor of the current solution.

SkyLeite · 2022-06-22T18:35:17Z

One more thing: I see a codeclimate issue from GitHub, but it seems I don't have access to the Details page to see what the issue is. Do we have a way of running these checks locally?

SkyLeite added 6 commits June 20, 2022 22:52

Use vertical line as drop target indicator

eb4f760

Darken column when it is being dragged

63f5d20

Fix not being able to drag to first or last slot

6eed1ef

Merge branch 'main' into comparison-table-improved-drag

f5ba4bc

Allow consumer to specify whether dragged element should be hidden

bcb9c0d

Adapt tests to new drag-drop mechanism for plots

ba65dad

SkyLeite added 3 commits June 21, 2022 22:32

Fix leftover state when dropping on an invalid target

ea20c18

Fix eslint error

a67b9b9

Restore original behavior for trends and data series

ba9c284

SkyLeite marked this pull request as ready for review June 22, 2022 02:49

SkyLeite requested review from mattseddon, rogermparent, sroy3 and wolmir as code owners June 22, 2022 02:49

rogermparent reviewed Jun 22, 2022

View reviewed changes

Compare elements by reference in drop target tests

ef35dc0

sroy3 reviewed Jun 22, 2022

View reviewed changes

SkyLeite added 6 commits June 22, 2022 10:30

Remove unnecessary display: block for children of dropTarget

5f5d0cb

Use ternary for improved readability

b41b5b8

Restore original tests for trends and data series

525465c

Revert behavior changes of trends and data series

e2e8caf

Fix drop target indicator showing as a double border on first and las…

31aebcf

…t items

Add styling to the ghost image of dragged plot headers

0887cc0

Fix invalid DOM content warnings

42f369d

SkyLeite requested review from sroy3 and rogermparent June 23, 2022 16:40

mattseddon closed this Jun 28, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use vertical line as drop target indicator for plots #1935

Use vertical line as drop target indicator for plots #1935

SkyLeite commented Jun 21, 2022 •

edited

Loading

sroy3 commented Jun 21, 2022

sroy3 commented Jun 21, 2022 •

edited

Loading

SkyLeite commented Jun 22, 2022

rogermparent commented Jun 22, 2022

rogermparent left a comment

rogermparent Jun 22, 2022

sroy3 Jun 22, 2022

SkyLeite Jun 22, 2022

rogermparent Jun 22, 2022

SkyLeite Jun 22, 2022

rogermparent Jun 22, 2022

SkyLeite Jun 22, 2022

rogermparent Jun 22, 2022

SkyLeite Jun 22, 2022

sroy3 left a comment

sroy3 Jun 22, 2022

SkyLeite Jun 22, 2022

SkyLeite commented Jun 22, 2022

SkyLeite commented Jun 22, 2022

SkyLeite commented Jun 22, 2022

Use vertical line as drop target indicator for plots #1935

Use vertical line as drop target indicator for plots #1935

Conversation

SkyLeite commented Jun 21, 2022 • edited Loading

sroy3 commented Jun 21, 2022

sroy3 commented Jun 21, 2022 • edited Loading

SkyLeite commented Jun 22, 2022

rogermparent commented Jun 22, 2022

rogermparent 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

Choose a reason for hiding this comment

sroy3 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SkyLeite commented Jun 22, 2022

SkyLeite commented Jun 22, 2022

SkyLeite commented Jun 22, 2022

SkyLeite commented Jun 21, 2022 •

edited

Loading

sroy3 commented Jun 21, 2022 •

edited

Loading