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

Spike: Look into making EuiDataGrid columns draggable #7943

Closed
tkajtoch opened this issue Aug 5, 2024 · 11 comments
Closed

Spike: Look into making EuiDataGrid columns draggable #7943

tkajtoch opened this issue Aug 5, 2024 · 11 comments
Assignees

Comments

@tkajtoch
Copy link
Member

tkajtoch commented Aug 5, 2024

Summary

A prep work for #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
Copy link
Contributor

mgadewoll commented Aug 12, 2024

🗒 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:

  • add a prop on EuiDataGrid to toggle the behavior, if the drag functionality should be conditional
  • implement EuiDragDropContext, EuiDroppable and EuiDraggable
  • reuse switchColumnPos to reorder columns on drag end
  • ensure focus context is correctly updated on drag end
  • ensure that the focus context keyboard navigation does not interfere with the drag keyboard navigation (arrowLeft/Right are used by both)
  • when drag behavior is enabled, focus the header cell instead of the actions button to ensure expected keyboard behavior (requires update of labelling the cell with the actions button to ensure proper screen reader output)
  • trigger the actions button on Enter only as DragStart and DragEnd are handled via Space (would this be a slight accessibility concern as buttons trigger on Enter and Space? But we're not actually on the button but on the cell. We need to ensure to announce key functionality)
  • add a visual indication for draggable headers (similar to appearing actions button?)

Image

mouse behavior
Image

⌨ Suggested keyboard navigation:

  • SPACE on a header cell starts dragging
  • while dragging, ARROW_LEFT and ARROW_RIGHT select the new droppable position
  • SPACE on a dragged header cell ends dragging on the selected droppable position
  • ESCAPE on a dragged header cell aborts the dragging process and the cell is returned to it's original position
  • ARROW_LEFT/ARROW_RIGHT on a focused header cell navigate to the previous/next header cell
  • ENTER on a focused, draggable header cell without interactive children but with actions button opens the actions menu
  • ENTER on a focused, draggable header cell with interactive children (with or without actions button) enters the cell
  • ESCAPE on an entered focused header cell with interactive children exits the cell

keyboard behavior
Image

@mgadewoll
Copy link
Contributor

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

@ryankeairns
Copy link
Contributor

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

cee-chen commented Aug 12, 2024

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

@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
Copy link
Contributor

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

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

mgadewoll commented Aug 14, 2024

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

mgadewoll commented Aug 14, 2024

@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
Copy link
Contributor

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

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants