-
Notifications
You must be signed in to change notification settings - Fork 829
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
[EuiDataGrid] Implement draggable column headers #8015
base: main
Are you sure you want to change the base?
[EuiDataGrid] Implement draggable column headers #8015
Conversation
@@ -110,6 +115,8 @@ export const EuiDraggable: FunctionComponent<EuiDraggableProps> = ({ | |||
role={ | |||
hasInteractiveChildren | |||
? 'group' | |||
: customDragHandle === 'custom' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The idea here is to provide means to unset the role
for specific use cases. Using customDragHandle=true
for that would have impact on current usages, hence I'm suggesting to add a third custom option as this should not be a default.
The reasoning here is this:
EuiDraggable adds a drag wrapper around its content, which has role="button|group"
- this works for most cases where the drag element should be the interactive/focused element. The impact of this is, that the content of this element is not read as standalone semantic elements. E.g. role="button"
removes additional semantic content and just reads visible text content.
For datagrid column headers we rather want the columnheader
to still be the element to be read via screen readers as it holds all semantic information. Instead we want the draggable accessible information added to the column header instead of the wrapper being read only.
To ensure the columnheader
role element is read, the draggable wrapper needs to have no role that removes the content semantics. Hence the decision to provide means to unset it for this case.
9e23fdd
to
2ccfb4d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All VRT images updates are related to this change which means the "Account" title now sits flush within the header cell padding instead of 2px offset due to the gap (which applied because the action elements are only visually hidden but available in the DOM)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we see an example of a draggable column header with interactive children? a la https://eui.elastic.co/storybook/?path=/story/tabular-content-euidatagrid--custom-header-content?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I updated the example to have some interactive columns to show both cases (commit)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! Linking the Storybook setup here just for helpfulness: https://eui.elastic.co/pr_8015/storybook/?path=/story/tabular-content-euidatagrid--custom-header-content&args=columnDragDrop:!true
To be totally honest, something about it feels a little a little off to me. Going to CC @MichaelMarcialis into this because I'm getting into UX designer territory:
It feels odd to me that the column actions have such a relatively smaller hitbox / interaction area, but the drag action has such a large one (literally the entire cell except for interactive children). It feels disproportionate in terms of UX emphasis - while reordering is an important feature, sorting a column is arguably even more important for tabular data and is now relatively harder to find/click on.
How would we feel about moving the draggable interaction area to just the handle on hover, instead of the entire cell backdrop? That way in terms of clickability, the drag handle, actions popover, and tooltip children all have roughly equal discoverability and one doesn't eclipse the other.
Am I way off in that thinking?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the ping, @cee-chen. It's a good question. Personally, I'm a bit torn between the two directions. There are pros and cons to both.
In my own personal experiences, I've seen it done both ways, though the majority of those experiences (Google Sheets, AirTable, etc.) have the entire header cell as a draggable zone, rather than relegating the action to a smaller drag handle. This extra draggable real estate is nice to bring the reordering of columns feature to the forefront. However, it could potentially be annoying if misclicks on other actions in the cell result in inadvertent drag actions.
On the other hand, relegating the drag to the drag handle would prevent the misclick issue. But at the same time, it dramatically reduces the drag real estate and may cause frustration for users attempting to drag the column when the cursor is not resting on the handle.
For my part, in playing around with the current approach in Storybook, I quite like it and don't find it particularly odd or frustrating to work with (even when interacting with the additional cell actions). I'd say lets proceed as is and just monitor for feedback to the contrary. In case anyone else has any opinions, I'll CC @elastic/platform-design to see if there are opposing thoughts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems fine, but I suggest we test this in Kibana > Discover before merging.
With the One Discover work, there are more and more extension points and some can involve actions and tooltips in the column headers. Poking around in Discover's data grid - for each solution - seems like a smart if not necessary thing for us to do.
@@ -0,0 +1,2 @@ | |||
- Added `columnDragDrop` prop on `EuiDataGrid` which enables reordering columns via draggable header cells |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm tempted to nest this under an existing setting rather than adding yet another top level prop to EuiDataGrid. How do you feel about gridStyle: { canDragDropColumns: true }
?
Or maybe the columnVisibility
prop, since that does affect column order? e.g.
columnVisibility={{
visibleColumns: [...],
setVisibleColumns: () => {},
canDragAndDropColumns: true,
}}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think gridStyle
might work, considering it also holds stickyFooter
.
columnVisibility
seems a bit too specific to visibility to add drag behavior? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
columnVisibility
actually does make sense to me given its prop description (emphasis mine):
Defines which columns are intitially visible in the grid and the order they are displayed
I'm also assuming we can't drag and drop leading/trailing control columns, correct? If so, columnVisibility
actually makes that clearer because it does not include control columns in its logic/array (whereas gridStyle
affects all columns).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, you're right. It does not apply to leading/trailing columns indeed.
I'll update to move the prop to columnVisibility
then 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated in this commit
packages/eui/src/components/datagrid/body/header/data_grid_header_cell.styles.ts
Outdated
Show resolved
Hide resolved
packages/eui/src/components/datagrid/body/header/data_grid_header_cell.styles.ts
Outdated
Show resolved
Hide resolved
packages/eui/src/components/datagrid/body/header/data_grid_header_row.styles.ts
Show resolved
Hide resolved
packages/eui/src/components/datagrid/body/header/data_grid_header_row.tsx
Outdated
Show resolved
Hide resolved
packages/eui/src/components/datagrid/body/header/data_grid_header_cell.tsx
Outdated
Show resolved
Hide resolved
// Draggable prevents FocusTrap onOutsideClick to be called. | ||
// We manually close the popover for draggable cells and | ||
// update the focus index onBlur to ensure execution order | ||
// as closePopover() focuses its own cells first on close. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I need to run for today but I'm planning on coming back to this file and doing a closer review tomorrow (also delaying because I'm guessing this might change significantly if we need to support #8015 (comment)).
Just curious, do we know exactly why the draggable library interferes with our existing click events?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interactive children work as expected with the draggable header columns (example story).
The general problem is that clicking the draggable column will click the draggable wrapper (and does not receive focus), not the column header. And somewhere the focus is caught, because the focus
listener on the header cell that handles the focus context does not trigger.
This starts happening as soon as the provided.draggableProps
are added. I did not find the root cause in code so far. 🤔
- draggable cells prevent onOutsideClick to be triggered, we need to manually update focus to ensure expected behavior - moves columnResizer element to ensure drag and resize actions stay separate
- add columnDragDrop control to custom ehader story for testing with interactive headers
- ensures the columnheader element is read instead of the wrapping draggable container; this requires the draggable wrapper to not have a role as the default roles button/group remove any semantics from their content when focused which results in the content not fully being read
- this reparenting approach is required due to transform context of datagrid which interfers with the positioning of dragged items
- use unique ids - remove position style override as it's not needed for the reparented/portalled approach
- the changes are only related to the conditionally added gap on header cells
- prevents error about not finishing drop animation as there were duplicate elements being dragged
- prevents duplicate SR output in entering the cell
…e elements - ensure we use the rowing tabindex and don't add additional unwanted tab stops
- includes column header with interactive cell content
141c294
to
deceb15
Compare
💚 Build Succeeded
History
|
Summary
closes #7136
This PR implements reordering
EuiDataGrid
columns via draggable column headers.This implementation is opt-in keeping parity with current usages.
Todo
EuiDataGrid
Emotion conversionadd docs update to EUI+ docs (DataGrid content is not yet available, EUI+ is still in need to be fully migrated/released)Changes
columnDragDrop
onEuiDataGrid
to toggle draggable column headers - default value isfalse
EuiDragDropContext
andEuiDroppable
inEuiGridHeaderRow
to enable a drop zone for all non-control columnsEuiDraggable
inEuiDataGridHeaderCell
to enable draggable column headersposition: fixed
element inside a transform contextcustomDragHandle
prop value onEuiDraggable
to beboolean | 'custom'
Accessibility
The
Draggable
implementation comes with great accessibility out of the box (adding hints and announcements where needed) BUT for the use case of datagrid headers it was not completely fitting and needed a small adjustment.The expected behavior is, that focussing a column header, we want the
columnheader
role to be read with all its expected labels and attached descriptions.EuiDraggable
currently always added a role on the drag container (button
orgroup
) which will be read instead of the content (effectively losing us semantic information). To solve this we optionally remove the added role on the drag container, which results in the content being read on focus as wanted.This was tested and optimized on Win11 for JAWS and NVDA.
Screen reader output
Screenshots
mouse usage
Screen.Recording.2024-09-10.at.18.01.57.mov
keyboard usage
Screen.Recording.2024-09-10.at.18.05.00.mov
resizing
Screen.Recording.2024-09-10.at.18.06.38.mov
QA
Storybook: https://eui.elastic.co/pr_8015/storybook/?path=/story/tabular-content-euidatagrid--playground&args=columnDragDrop:!true
EUI docs: https://eui.elastic.co/pr_8015/index.html#/tabular-content/data-grid-schema-columns#draggable-columns
columnDragDrop
prop works to en/disable draggable columnsGeneral checklist
Checked in both light and dark modes@default
if default values are missing) and playground togglesChecked Code Sandbox works for any docs examplesIf applicable, added the breaking change issue label (and filled out the breaking change checklist)If applicable, file an issue to update EUI's Figma library with any corresponding UI changes. (This is an internal repo, if you are external to Elastic, ask a maintainer to submit this request)