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

[jughosta]

• Closes [OneDiscover] Enable columns sorting by draggable column header #195769

Summary

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

Checklist

• Unit or functional tests were updated or added to match the most common scenarios

[jughosta]

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

[davismcphee]

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;
}

[jughosta]

Great idea, thanks! Updated via 7946301

[elasticmachine]

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

[davismcphee]

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.

[davismcphee]

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

[davismcphee]

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;
}

[davismcphee]

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

[ryankeairns]

nit :)
This could be...

padding-block: $euiSizeXS / 2;

[jughosta]

Updated via 7967c37, thanks!

[ryankeairns]

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

Excited for the feature :)

[logeekal]

Thanks for making this change opt-in 🙏 . I have desk tested it and table is working fine.

@jughosta , @davismcphee, I would suggest that these tickets should take into account ( during estimation ) the compatibility of consumers of unified data table ( in this case security solution ) otherwise, it increases tech debt for us since there are multiple features as of now which are opt-in such as this one and couple of previous deprecations of externalControlColumns.

[davismcphee]

Thanks @logeekal for working through this with us.

@jughosta , @davismcphee, I would suggest that these tickets should take into account ( during estimation ) the compatibility of consumers of unified data table ( in this case security solution ) otherwise, it increases tech debt for us since there are multiple features as of now which are opt-in such as this one and couple of previous deprecations of externalControlColumns.

You might need to expand on this point a bit because I'm not sure I understand. Some features we intentionally make opt-in, like the document comparison feature, since we can't be sure all consumers would want it enabled by default. Other features, like this one, would ideally be enabled by default since they should be appropriate for all consumers, but we're making it opt-in for now since it happens to have a bug when used in Timeline (and I appreciate you checking btw since I did forget to check Timeline originally).

For deprecations like externalControlColumns, we are maintaining the deprecated APIs until consumers are able to get off them, and have created a followup issue for the Timeline work: #189294. Sometimes we're able to address these directly in consumers' code during the initial PR if the work is straightforward, but in other cases we're unfamiliar with the code and it would be more feasible for consuming teams to address it when they can prioritize. Do you feel there is something more here we could be doing?

[logeekal]

@davismcphee , @jughosta ,

Firstly, I just want to make it clear that please do not consider this discussion as a blocker to this PR. I think we can safely merge it from investigations team perspective.

Some features we intentionally make opt-in, like the document comparison feature, since we can't be sure all consumers would want it enabled by default.

100% agree with the above point.

Other features, like this one, would ideally be enabled by default since they should be appropriate for all consumers, but we're making it opt-in for now since it happens to have a bug when used in Timeline (and I appreciate you checking btw since I did forget to check Timeline originally).

Yes, my opinion was mainly regarding these kind of features and it is just an opinion so feel free to disagree. I was mainly referring to keeping consumer's code either up-to-date for opt-in features or identification of bug ( reason why code cannot be updated ) should be part of the change of the shared code. An example for the same is that you were able to find out the issue with EUI and EuiDataGrid in a modal. Thank you for that by the way 👏 👏 . I looked into it yesterday whole day and could not have guessed it was a modal issue.

Do you feel there is something more here we could be doing?

My statement was not just for your team. It is applicable to us as well when we are making a change in a shared component such as unified-data-table or anything else.

Sometimes we're able to address these directly in consumers' code during the initial PR if the work is straightforward, but in other cases we're unfamiliar with the code and it would be more feasible for consuming teams to address it when they can prioritize.

I appreciate that unfamilar code can be time taking and I think we should be able to put in some time to take a look if nothing breaks. I was mainly concerned about 2 things and may be it was a pre-cautionary comment.

1. If a change is a deprecation, if possible, the consuming could should be updated. But i also agree with you that if change is big for consumers, it should be left of them.
2. If a change is opt-in, testing that opting-in does not break consumer's code ( like discover and security solution ) should also be part of ticket. And case-by-case basis we can decide whether it is worth doing now to later as a separate work.

Sometimes we're able to address these directly in consumers' code during the initial PR if the work is straightforward

I think this statement really captures the essence of what I was saying and i think we are on the same page. When making that comment, I mainly had deprecations and this particular change ( which breaks security solution when opted in ) in mind. Both of these are now tech-debt for us and somehow lose priority ( I know that is our problem ) and I was feeling that cumulatively, we can be behind a lot of functionality.

[kertal]

Other features, like this one, would ideally be enabled by default since they should be appropriate for all consumers, but we're making it opt-in for now since it happens to have a bug when used in Timeline (and I appreciate you checking btw since I did forget to check Timeline originally).

Yes, my opinion was mainly regarding these kind of features and it is just an opinion so feel free to disagree. I was mainly referring to keeping consumer's code either up-to-date for opt-in features or identification of bug ( reason why code cannot be updated ) should be part of the change of the shared code. An example for the same is that you were able to find out the issue with EUI and EuiDataGrid in a modal. Thank you for that by the way 👏 👏 . I looked into it yesterday whole day and could not have guessed it was a modal issue.

I do disagree. This kind of bug is not foreseeable, it can't be estimated. iI's a small change code wise, adapting a behavior in the EuiDataGrid that should be the default, and is long requested. So IMO having a code review from CodeOwners should have been sufficient, similar to other smaller changes we added to the table. Also we can't test all consuming apps. The modal problem is unfortunate, but such things happen in software engineering. 🙏 thx for catching it.

@logeekal Ideally this should have been caught by functional testing, because also moving left and right in the "old" way didn't work with this PR initially. What would be actionable to prevent something like this, would maybe to implement more essential functional tests of the table in the security timeline, and think about a way if we could deduplicate code longer down the road.

here's an example of the discover coverage:

kibana/test/functional/apps/discover/embeddable/_saved_search_embeddable.ts

Line 102 in 2b645d5

await dataGrid.clickMoveColumnLeft('agent');

On the other hand, there's OneDiscover, and this should won't surface in a modal, right, so it would benefit already by functional coverage of the table.

[elasticmachine]

💚 Build Succeeded

• Buildkite Build
• Commit: 4008bd8

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.1KB	509.3KB	+188.0B
discover	809.2KB	809.4KB	+212.0B
esqlDataGrid	153.5KB	153.7KB	+188.0B
securitySolution	21.0MB	21.0MB	+940.0B
slo	856.3KB	856.4KB	+189.0B
total			+1.7KB

Unknown metric groups

API count

id	before	after	diff
@kbn/unified-data-table	183	184	+1

History

• 💔 Build #248412 failed 940b492
• 💛 Build #248026 was flaky ae44093
• 💔 Build #247966 failed a8eaa1b
• 💚 Build #247700 succeeded 7967c37
• 💚 Build #247348 succeeded 979fa69

cc @jughosta

[kibanamachine]

Starting backport for target branches: 8.x

https://github.com/elastic/kibana/actions/runs/11703101653

[kibanamachine]

💚 All backports created successfully

Status	Branch	Result
✅	8.x

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation