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

[Discover][UnifiedDataTable] Enable drag&drop for grid columns #197832

Open
wants to merge 17 commits into
base: main
Choose a base branch
from

Conversation

jughosta
Copy link
Contributor

@jughosta jughosta commented Oct 25, 2024

Summary

Eui now supports reordering of grid columns by dra&drop elastic/eui#8015
The PR enables this functionality for UnifiedDataTable.

Oct-28-2024 14-30-11

Checklist

@jughosta jughosta added release_note:enhancement Team:DataDiscovery Discover App Team (Document Explorer, Saved Search, Surrounding documents, Data, DataViews) backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) labels Oct 25, 2024
@jughosta jughosta self-assigned this Oct 25, 2024
@@ -41,13 +41,17 @@
}

.euiDataGridHeaderCell {
align-items: start;
align-items: center;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

To align with the "graggable" state of the header column which is in a portal and we can't override it there.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can keep the original styles and have this work for dragging by adding this to our EuiDataGridColumn defs:

displayHeaderCellProps: { className: `unifiedDataTable__headerCell` }

Then we can remove these nested styles including euiDataGridHeaderCell__popover and euiDataGridHeaderCell__draggableIcon, and use a single global style:

.euiDataGridHeaderCell.unifiedDataTable__headerCell {
  align-items: start;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great idea, thanks! Updated via 7946301

@jughosta jughosta marked this pull request as ready for review October 28, 2024 15:32
@jughosta jughosta requested review from a team as code owners October 28, 2024 15:32
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-data-discovery (Team:DataDiscovery)

Copy link
Contributor

@davismcphee davismcphee left a comment

Choose a reason for hiding this comment

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

Looking good to me overall! Just a small suggestion around some styling.

It works well, although the delay when reordering is a bit annoying. But that exists in the EUI docs too for the most part, so not really something we can address on the Discover side. Interestingly there's less of a delay when using the keyboard and it feels smoother, maybe since there's no animation.

Comment on lines +105 to +122
const headerDraggableColumns = await this.find.allByCssSelector(
'[data-test-subj="euiDataGridHeaderDroppable"] > div'
);
// searching for a common parent of the field column header and its resizer
const fieldHeader: WebElementWrapper | null | undefined = (
await Promise.all(
headerDraggableColumns.map(async (column) => {
const hasFieldColumn =
(await column.findAllByCssSelector(`[data-gridcell-column-id="${field}"]`)).length > 0;
return hasFieldColumn ? column : null;
})
)
).find(Boolean);
const resizer = await fieldHeader?.findByTestSubject('dataGridColumnResizer');

if (!fieldHeader || !resizer) {
throw new Error(`Unable to find column resizer for field ${field}`);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Wow, maybe we should ask EUI for a test subj on the draggable wrappers as a followup 😅

@@ -41,13 +41,17 @@
}

.euiDataGridHeaderCell {
align-items: start;
align-items: center;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can keep the original styles and have this work for dragging by adding this to our EuiDataGridColumn defs:

displayHeaderCellProps: { className: `unifiedDataTable__headerCell` }

Then we can remove these nested styles including euiDataGridHeaderCell__popover and euiDataGridHeaderCell__draggableIcon, and use a single global style:

.euiDataGridHeaderCell.unifiedDataTable__headerCell {
  align-items: start;
}

Copy link
Contributor

@davismcphee davismcphee left a comment

Choose a reason for hiding this comment

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

And with the latest styling changes, this LGTM 👍 Thanks, it's so much nicer reordering columns this way vs the popover!

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
cloudSecurityPosture 509.7KB 509.8KB +151.0B
discover 828.6KB 828.7KB +151.0B
esqlDataGrid 153.5KB 153.6KB +151.0B
securitySolution 21.0MB 21.0MB +755.0B
slo 855.7KB 855.8KB +151.0B
total +1.3KB

History

cc @jughosta

align-items: start;

.euiDataGridHeaderCell__draggableIcon {
padding-block: 2px // to align with a token height
Copy link
Contributor

Choose a reason for hiding this comment

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

nit :)
This could be...

padding-block: $euiSizeXS / 2;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated via 7967c37, thanks!

Copy link
Contributor

@ryankeairns ryankeairns left a comment

Choose a reason for hiding this comment

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

Approving with a small nit to use an EUI variable in place of the 2px

Excited for the feature :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) release_note:enhancement Team:DataDiscovery Discover App Team (Document Explorer, Saved Search, Surrounding documents, Data, DataViews)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[OneDiscover] Enable columns sorting by draggable column header
4 participants