elastic / eui

Elastic UI Framework 🙌
https://eui.elastic.co/
Other
6.08k stars 829 forks source link

Spike: Look into making EuiDataGrid columns draggable #7943

Closed tkajtoch closed 3 weeks ago

tkajtoch commented 1 month ago

Summary

A prep work for https://github.com/elastic/eui/issues/7136

See if it's possible to implement this quickly and ensure keyboard navigation and overall accessibility requirements are met

Document findings in comments to this issue

mgadewoll commented 1 month ago

🗒 Looking at EuiDataGrid and it's functionality, my conclusion is that it's possible to update it to have draggable column headers.

[!IMPORTANT] The investigation was done considering this previous update #7898 to support interactive header content.

EuiDataGrid already offers functionality to reorder columns via switchColumnPos which is currently used in the columns actions menu and the toolbars columns selector popover. We can reuse this functionality in EuiDataGrid header.

🗺 There are a few considerations for this update:

Image

mouse behavior Image

⌨ Suggested keyboard navigation:

keyboard behavior Image

mgadewoll commented 1 month ago

@MichaelMarcialis Are there any design concerns here? Do we want to show an icon to indicate dragging behavior?

ryankeairns commented 1 month ago

This is rad! It was one of most - if not the top - observed ux 'issue' when we tested the data grid in Discover. Users expected it to work this way, and it was a downer each time they hit this part of the test.

cee-chen commented 1 month ago

Keyboard UX looks 👨‍🍳 💋 - very excited for that! We should make sure the SPACE keypress is clearly indicated to screen readers. Your flowchart of where to put draggable/droppable components also looks great.

add a prop on EuiDataGrid to toggle the behavior, if the drag functionality should be conditional

We generally have everything as a prop that can be optionally turned off. For this, I would suggest using the existing columns API to allow consumers to turn off the drag handle per-column. My suggestion would be to use the existing columns.actions config, e.g.

const columns = [{
  id: 'someColumn',
  actions: {
    showDragHandle: false,
    showMoveLeft: false,
    showMoveRight: false,
  },
}];

<EuiDataGrid columns={columns} />
mgadewoll commented 1 month ago

@1Copenut Do you have additional thoughts on this intended update? We can definitely announce that Space key starts dragging behavior, similar to how we announce Enter key to enter cells. Besides that there are already announcements about the drag update baked in by using our drag components.

MichaelMarcialis commented 1 month ago

This looks wonderful, @mgadewoll! Very well done!

Are there any design concerns here? Do we want to show an icon to indicate dragging behavior?

Which icon do you mean here? Do you mean the grab icon being used for a drag handle? Or the mouse cursor? Or something else? If you mean drag handle, I think @andreadelrio and I had suggested using grabOmnidirectional for data grid column reordering in some recent discover mockups. I suppose grabVertical could also work, but I think I prefer the omnidirectional version. For the mouse cursor, I think the current usages of grab and grabbing work fine.

Otherwise, no other major design concerns on my end. The only thing worth mentioning was the header offset that happens during the dragging event. I love the offset during the keyboard dragging event, but it feels slightly strange during the mouse dragging event. During a mouse drag, it looks like the user is grabbing the air above the heading, rather than the heading itself. Would it be possible to limit the offset to only the keyboard dragging event?

CleanShot 2024-08-12 at 14 38 54

cee-chen commented 1 month ago

During a mouse drag, it looks like the user is grabbing the air above the heading, rather than the heading itself. Would it be possible to limit the offset to only the keyboard dragging event?

Potentially related to dragging within a stacking context - the draggable area cannot have transforms etc sadly 😬

mgadewoll commented 1 month ago

During a mouse drag, it looks like the user is grabbing the air above the heading, rather than the heading itself. Would it be possible to limit the offset to only the keyboard dragging event?

While we can't change the transform I think we should be able to overwrite the top positioning that's added.

mouse Image

keyboard Image

mgadewoll commented 1 month ago

@MichaelMarcialis

Which icon do you mean here? Do you mean the grab icon being used for a drag handle?

I was referring to the drag icon iconType="grab" shown only on hover/focus on the left of each draggable cell. I used the same logic here to show it as the actions menu, but I wanted to align if we a) want to show anything and b) if we do, what should be the expected behavior for it.

MichaelMarcialis commented 4 weeks ago

While we can't change the transform I think we should be able to overwrite the top positioning that's added.

Thanks for that update, @mgadewoll! The mouse dragging looks great now. Is there any way we can limit the top positioning override to mouse drag events only? Asking because I think I preferred the keyboard drag the way you had it previously.

I was referring to the drag icon iconType="grab" shown only on hover/focus on the left of each draggable cell. I used the same logic here to show it as the actions menu, but I wanted to align if we a) want to show anything and b) if we do, what should be the expected behavior for it.

I think the current hover/focus behavior you have is perfect. The only thing I would suggest is changing iconType="grab" to iconType="grabOmnidirectional".

mgadewoll commented 3 weeks ago

@MichaelMarcialis

Is there any way we can limit the top positioning override to mouse drag events only?

To separate those styling overrides we'd need to keep track of the different event modes storing them in a state and triggering an additional rerender because of it. Imho, it's not worth that to just have a slightly different positioning 🙈