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.

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(dashboard): update tab drag and drop reordering with positional placement and indicators for UI #29395

Merged

Conversation

rtexelm
Copy link
Member

@rtexelm rtexelm commented Jun 28, 2024

SUMMARY

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

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

B:

tabs-reorder-old.mov

A:

tabs-reoder-new.mov

TESTING INSTRUCTIONS

  • Create or navigate to a dashboard with tabs
  • Drag tabs over other tabs to reorder

ADDITIONAL INFORMATION

  • Has associated issue:
  • Required feature flags:
  • 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
Copy link
Member Author

rtexelm commented Jun 28, 2024

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

@rtexelm rtexelm changed the title refactor(dashboard): update tab drag and drop reordering with positional placement and indicators for UI**DO NOT MERGE** feat(dashboard): update tab drag and drop reordering with positional placement and indicators for UI**DO NOT MERGE** Jun 28, 2024
@rtexelm rtexelm marked this pull request as ready for review July 19, 2024 12:55
@rtexelm rtexelm changed the title feat(dashboard): update tab drag and drop reordering with positional placement and indicators for UI**DO NOT MERGE** feat(dashboard): update tab drag and drop reordering with positional placement and indicators for UI Jul 19, 2024
@dosubot dosubot bot added change:frontend Requires changing the frontend dashboard:tab Related to the usage of tabs in the Dashboard labels Jul 19, 2024
@geido
Copy link
Member

geido commented Jul 19, 2024

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

@geido
Copy link
Member

geido commented Jul 19, 2024

/testenv up

@geido geido requested review from justinpark and kgabryje and removed request for justinpark July 19, 2024 15:31
@yousoph
Copy link
Member

yousoph commented Jul 22, 2024

/testenv up

@rtexelm rtexelm force-pushed the fix/update-tab-dragging-reordering/sc-76019 branch from a3dd2bc to 6601b1c Compare July 22, 2024 21:14
@yousoph
Copy link
Member

yousoph commented Jul 22, 2024

/testenv up

Copy link
Contributor

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

@geido
Copy link
Member

geido commented Jul 23, 2024

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
Copy link
Member

geido commented Jul 23, 2024

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.

@rtexelm
Copy link
Member Author

rtexelm commented Jul 25, 2024

@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.

@yousoph
Copy link
Member

yousoph commented Jul 25, 2024

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.

@geido
Copy link
Member

geido commented Jul 30, 2024

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.

@rtexelm
Copy link
Member Author

rtexelm commented Aug 5, 2024

@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 rtexelm force-pushed the fix/update-tab-dragging-reordering/sc-76019 branch from 8dfc058 to edce75f Compare August 5, 2024 18:38
@geido
Copy link
Member

geido commented Sep 9, 2024

/testenv up

@geido geido self-requested a review September 9, 2024 16:56
Copy link
Contributor

github-actions bot commented Sep 9, 2024

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

@rtexelm rtexelm force-pushed the fix/update-tab-dragging-reordering/sc-76019 branch from edce75f to 9f45843 Compare September 13, 2024 19:35
@geido
Copy link
Member

geido commented Sep 16, 2024

/testenv up

Copy link
Contributor

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

Copy link
Member

@geido geido left a comment

Choose a reason for hiding this comment

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

There might be some clean up possible for the way state is handled, but I think this looks good. Looking to get some feedback from @kgabryje too

@kgabryje
Copy link
Member

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

Screen.Recording.2024-09-19.at.14.15.50.mov

Copy link
Member

@geido geido left a comment

Choose a reason for hiding this comment

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

Due to @kgabryje's comment. I still continue to suspect that state management might be the issue here.

@yousoph
Copy link
Member

yousoph commented Sep 20, 2024

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

@kgabryje
Copy link
Member

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

@geido
Copy link
Member

geido commented Sep 27, 2024

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

@geido
Copy link
Member

geido commented Sep 30, 2024

@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 geido merged commit bdd50c7 into apache:master Sep 30, 2024
34 checks passed
Copy link
Contributor

Ephemeral environment shutdown and build artifacts deleted.

@rtexelm
Copy link
Member Author

rtexelm commented Oct 8, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
change:frontend Requires changing the frontend dashboard:tab Related to the usage of tabs in the Dashboard size/L
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants