superset icon indicating copy to clipboard operation
superset copied to clipboard

feat(dashboard): update tab drag and drop reordering with positional placement and indicators for UI**DO NOT MERGE**

Open rtexelm opened this issue 1 year ago • 1 comments

SUMMARY

Change reordering and UI specifically for Tab components within Tabs parent components across the codebase.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

  • [ ] Has associated issue:
  • [ ] Required feature flags:
  • [x] Changes UI
  • [ ] Includes DB Migration (follow approval process in SIP-59)
    • [ ] Migration is atomic, supports rollback & is backwards-compatible
    • [ ] Confirm DB migration upgrade and downgrade tested
    • [ ] Runtime estimates and downtime expectations provided
  • [ ] Introduces new feature or API
  • [ ] Removes existing feature or API

rtexelm avatar Jun 28 '24 06:06 rtexelm

Still need to add/edit the tests and refine some things but this is the general idea.

rtexelm avatar Jun 28 '24 15:06 rtexelm

Let's include a video of before and after, please. Thank you

geido avatar Jul 19 '24 15:07 geido

/testenv up

geido avatar Jul 19 '24 15:07 geido

/testenv up

yousoph avatar Jul 22 '24 16:07 yousoph

/testenv up

yousoph avatar Jul 22 '24 22:07 yousoph

@yousoph Ephemeral environment spinning up at http://35.87.176.169:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

github-actions[bot] avatar Jul 22 '24 22:07 github-actions[bot]

A couple of visual feedback:

  • When dragging the "x" that closes the tab stays in the original position of the tab, shouldn't that be hidden when dragging or dragged across with the tab?
  • When trying to drop on the right side of a tab, it does not catch the drop indicator until you hover the title of the tab, which is inconvenient
  • The title of the tab is being highlighted in addition to the drop indicator, is that necessary?

I know some of these things were harder to implement but I feel these are still quite relevant. Wdyt @yousoph?

geido avatar Jul 23 '24 11:07 geido

Also, not sure if this worked like that before, but when you try to drag a selected tab, it will auto-select the next available tab on the right.

Another issue is that as you drag and target a new tab, it will make the target tab selected.

These create a lot of unnecessary movement of the tabs and the content which can be resource-intensive when you have a bunch of charts on top of being undesirable in terms of UX.

geido avatar Jul 23 '24 11:07 geido

@geido @yousoph Removing the tab respective X (delete) icon from the from the tabs bar while dragging posed a disproportionately complex problem due to the antd structure of the closeIcon prop. Using the isDragging state in it's current form to change the styling of the tab being dragged will not update to false, so the internal state of the tabs component keeps the tab un-rendered until a different tab is dragged. Because the DragDroppable is only able to wrap the tab title I added the CSS to remove that component but it can be set back so the tab title is at 20% opacity instead.

The necessity to hover over the tab title is another result of only wrapping that component in the DragDroppable.

Tab title highlighting during drag over can be changed, it was just the default behavior.

All of the tab activation behavior, i.e. the active tab changes to that which is hovered over, is the unchanged previous behavior. Wasn't sure if it was in scope to alter that as it is also the behavior of dragging charts into tabs.

rtexelm avatar Jul 25 '24 16:07 rtexelm

I think it would be good to remove the title highlighting if possible!

The active tab behavior looked ok to me - I think it's fine to keep it as it was.

yousoph avatar Jul 25 '24 17:07 yousoph

I think it would be good to remove the title highlighting if possible!

The active tab behavior looked ok to me - I think it's fine to keep it as it was.

I still think that the "x" staying at the original place is weird and should probably not happen.

geido avatar Jul 30 '24 15:07 geido

@geido @yousoph I tried a different approach here that I was reluctant to in past efforts. I had assumed the dnd configs would break for other dashboard elements if I changed them overtly but the changes here have been pretty innocuous. Now tabs should vanish during drag and the title drop indicator should render only if the dragObject is not a Tab.

rtexelm avatar Aug 05 '24 02:08 rtexelm

/testenv up

geido avatar Sep 09 '24 16:09 geido

@geido Ephemeral environment spinning up at http://54.70.182.123:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

github-actions[bot] avatar Sep 09 '24 17:09 github-actions[bot]

/testenv up

geido avatar Sep 16 '24 17:09 geido

@geido Ephemeral environment spinning up at http://34.221.129.187:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

github-actions[bot] avatar Sep 16 '24 17:09 github-actions[bot]

It look kinda weird to me that the dragged tab disappears and other tabs become active when I drag over them. Also it's working kinda slowly - it's not smooth at all on my m2pro mac, so it would be really bad on older computers.

CC @yousoph

https://github.com/user-attachments/assets/434a858f-62d2-40a5-b43f-139155fa009f

kgabryje avatar Sep 19 '24 13:09 kgabryje

re: the active tab changing, I think that is already happening on the current version of tab dragging (before this PR)

yousoph avatar Sep 20 '24 17:09 yousoph

re: the active tab changing, I think that is already happening on the current version of tab dragging (before this PR)

Oh you're right! I haven't noticed that before. Still though, I like that this PR adds the indicators of where the dragged tab is going to land, but I'm not sure about the dragged tab disappearing from its original spot. Now when we drag a tab, there's a "ghost" (low opacity) left. But it's not a blocker if you think it looks better now

kgabryje avatar Sep 23 '24 16:09 kgabryje

@kgabryje I am not experiencing the slowness issue. If this is a go from @yousoph I think we might let this go.

geido avatar Sep 27 '24 14:09 geido

@yousoph could not repro the slowness and so could not QA, so I guess we are good. Merging this now and keeping an eye just in case someone should report a similar problem as @kgabryje did. In that case, we should look at how to improve state management to avoid excessive re-rendering during drag and drop.

geido avatar Sep 30 '24 09:09 geido

Ephemeral environment shutdown and build artifacts deleted.

github-actions[bot] avatar Sep 30 '24 09:09 github-actions[bot]

Just wanted to drop a thanks for moving this forward while I've been away on hiatus. Let me know if there's any complications I can help with itf

rtexelm avatar Oct 08 '24 17:10 rtexelm