[OneDiscover][Extension] DataTable Row Actions #188762

jughosta · 2024-07-19T15:15:55Z

Summary

Extend UnifiedDataTable with a new rowAdditionalLeadingControls prop to render additional leading controls
In case of many actions, collapse the rest of them under "More" button
New OneDiscover extension
Convert from customControlColumnsConfiguration to the new prop. Refactor actions format in Log Explorer.
Swap the default "select" and "expand" control columns ~~if custom row actions are specified~~
Add to example OneDiscover profile
Add functional and units tests

Testing

For testing the example profile:

add discover.experimental.enabledProfiles: ['example-root-profile', 'example-data-source-profile', 'example-document-profile'] to kibana.dev.yml
start kibana
make sure to have an index with my-example-logs name or create an alias to an existing index:

POST _aliases
{
  "actions": [
    {
      "add": {
        "index": "kibana_sample_data_logs",
        "alias": "my-example-logs"
      }
    }
  ]
}

create a data view for my-example-logs index

Follow up for Security solution

The following items would require deprecation/refactoring in components on Security Solution pages to have consistent UX (can result in 500+ lines of code changes):

Convert from externalControlColumns to the new prop rowAdditionalLeadingControls
~~Convert from trailingControlColumns to a normal column. trailingControlColumns is deprecated~~
- [OneDiscover][Security Solution] Refactor Security components to use rowAdditionalLeadingControls prop for UnifiedDataTable #189294
Use getRowIndicator prop on UnifiedDataTable instead of border-left style
- [OneDiscover][Security Solution] Refactor Security components to use getRowIndicator prop for UnifiedDataTable #189295

Checklist

Unit or functional tests were updated or added to match the most common scenarios
This renders correctly on smaller devices using a responsive layout. (You can test this in your browser)
This was checked for cross-browser compatibility

…ns` props

jughosta · 2024-07-24T15:51:20Z

/ci

jughosta · 2024-07-25T08:45:37Z

/ci

jughosta · 2024-07-25T12:48:24Z

/ci

jughosta · 2024-07-25T14:46:10Z

/ci

jughosta · 2024-07-25T16:24:53Z

/ci

jughosta · 2024-08-01T14:02:58Z

Swapped the default row controls in this PR via 154eeac and closed #188908

kertal · 2024-08-01T14:03:05Z

Should we swap the default controls in this PR? I'm asking again as we are not continuing with #186808

I'd be ok with swapping them in this PR, but there were some concerns raised last time we planned to make changes to these controls. I think we should either get O11y and Security to confirm they're ok with it or make the change in a followup.

+1 one from my end, I also thing we should swap them in the PR, but raising awareness about this is a good strategy

asking here: #188908 (comment) we can of course discuss in the sync, in necessary

as discussed async and in sync, no objections to permanently swap the positions

davismcphee

Did a quick test of the latest changes to update control column order and it's working as expected, thanks! 🙏

Heenawter

Update to embed mode screenshot test baseline LGTM 👍 I scanned the PR, and I think that is the only change owned by the @elastic/kibana-presentation team... For some reason the codeowners file filter isn't available 🙈

…extension # Conflicts: # src/plugins/discover/public/context_awareness/profile_providers/example_data_source_profile/profile.tsx # src/plugins/discover/public/context_awareness/types.ts

tonyghiani

Overall LGTM, although I noticed from the screenshot of the Logs Explorer in the description that the actions label disappeared from the column header.

The question mark icon alone feels a bit off, is this made on purpose?

jughosta · 2024-08-05T13:14:34Z

Hi @tonyghiani,
Thanks for the review!

The new actions are positioned each in its own grid control cell now. This allows for better keyboard navigation between them but yes, the header cell does not have enough space for the label now. Is it problematic if we drop it?

logeekal

Thanks for these changes. Left a question/comment regarding trailingControlColumn which I think should be not be deprected.

Use getRowIndicator prop on UnifiedDataTable instead of border-left style

This change is helpful. But I think it might conflict ( will have to test ) with Security solution's Event Renderers which needs color indicators in 2 lines for one row as shown below

Currently it is simply a css based border styling and in my opinion, making it a seperate cell, makes it somewhat difficult for us to add color to the extra Event Rendered row.

May i know what was reason it is not a style (eg, border-left) but a seperate cell?

packages/kbn-unified-data-table/src/components/data_table.tsx

logeekal · 2024-08-05T11:48:23Z

...mponents/custom_control_columns/additional_row_control/get_additional_row_control_columns.ts

+export const getAdditionalRowControlColumns = (
+  rowControlColumns: RowControlColumn[]
+): EuiDataGridControlColumn[] => {
+  if (rowControlColumns.length <= 2) {


It is a nit but Do we need a configuration on how many controls are visible? I will add @paulewing to this conversation because security uses 2 controls before shifting others in the popover menu.

The implementation is according to the provided design. For consistency, I would not introduce a custom configuration for items visible/hidden. We can still increase this number of visible actions as a follow up if becomes necessary.

Sure. Thanks for your response.

Just to clarify, here is the current design where we have space for 3 controls (including the default expand control):

logeekal · 2024-08-05T11:57:45Z

packages/kbn-unified-data-table/src/components/data_table_row_control.tsx

+  normal = 'normal',
+}
+
+export const DataTableRowControl: React.FC<{ size?: Size }> = ({ size, children }) => {


Shouldn't size default to Size.normal?

Good question. I think for now it makes sense to apply a fixed size only for actions we provide via UnifiedDataTable and keep the default as it was before. In Security, there are larger control cells.

tonyghiani · 2024-08-05T13:36:17Z

The new actions are positioned each in its own grid control cell now. This allows for better keyboard navigation between them but yes, the header cell does not have enough space for the label now. Is it problematic if we drop it?

Not a big problem, but only the icon feels a bit confusing, I'd rather say we go without any icon/header label at this point.

tonyghiani

LGTM

jughosta · 2024-08-06T14:09:04Z

Hi @logeekal,

Thanks for the review!

May i know what was reason it is not a style (eg, border-left) but a seperate cell?

There are several reasons:

border-left affects cells dimension and pushes the content to the side, it's noticeable on your screenshot too
for setting custom styles we would have to use rowClasses prop on EuiDataGrid. Defining this value would require iterating through all rows at once. From performance perspective, separate cell control allows to determine current row color only if row becomes visible when scrolling. So it scales better regardless how much data was fetched.
getRowIndicator provides a convenient way for consumers of UnifiedDataTable to dynamically assign colors to row indicators.

But I think it might conflict ( will have to test ) with Security solution's Event Renderers which needs color indicators in 2 lines for one row as shown below

Please let us know whether it would conflict or not. It might be possible to reuse ColorIndicatorCell as additional control on the second line or assign a border for the second line only. Also, Summary column could be more suitable when having a custom event renderer row. I don't see it being a blocker for this PR.

logeekal

Thank you @jughosta for all the clarifications. PR Looks great and I have desk tested as well. As you already said that if at a later stage, changes are needed we can do so at that point.

logeekal · 2024-08-07T12:44:33Z

Please let us know whether it would conflict or not. It might be possible to reuse ColorIndicatorCell as additional control on the second line or assign a border for the second line only. Also, Summary column could be more suitable when having a custom event renderer row. I don't see it being a blocker for this PR.

Honestly, I do not know yet. But I think we can tackle it later if that problem arises. Thanks for explaining the reasoning.

kibana-ci · 2024-08-07T13:26:38Z

💚 Build Succeeded

Buildkite Build
Commit: e8f6fb8
Kibana Serverless Image: docker.elastic.co/kibana-ci/kibana-serverless:pr-188762-e8f6fb81871b

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`cloudSecurityPosture`	694	699	+5
`discover`	976	981	+5
`esqlDataGrid`	408	413	+5
`lens`	1495	1500	+5
`logsExplorer`	610	612	+2
`securitySolution`	5610	5615	+5
`slo`	889	894	+5
total			+32

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id	before	after	diff
`@kbn/unified-data-table`	81	92	+11

Async chunks

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

id	before	after	diff
`cloudSecurityPosture`	464.4KB	467.2KB	+2.8KB
`discover`	835.5KB	839.0KB	+3.6KB
`esqlDataGrid`	122.5KB	125.2KB	+2.8KB
`logsExplorer`	247.2KB	223.5KB	-23.7KB
`securitySolution`	20.5MB	20.5MB	+13.8KB
`slo`	908.8KB	911.6KB	+2.8KB
total			+1.9KB

Public APIs missing exports

Total count of every type that is part of your API that should be exported but is not. This will cause broken links in the API documentation system. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats exports for more detailed information.

id	before	after	diff
`@kbn/unified-data-table`	1	2	+1

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id	before	after	diff
`logsExplorer`	28.2KB	27.7KB	-455.0B

Unknown metric groups

API count

id	before	after	diff
`@kbn/unified-data-table`	154	166	+12

async chunk count

id	before	after	diff
`logsExplorer`	6	5	-1

References to deprecated APIs

id	before	after	diff
`cloudSecurityPosture`	5	6	+1
`securitySolution`	473	474	+1
total			+2

History

💚 Build #226031 succeeded 499ab67
💚 Build #226000 succeeded 78fd61a
💚 Build #225753 succeeded c5bb1ef
💚 Build #225423 succeeded b058278
💔 Build #225385 failed 4edef40

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

cc @jughosta

## 📓 Summary Closes #190457 This change extracts the logic around the logs document controls/indicator from Logs Explorer and registers the same controls in Discover through the extension point added in #188762. The controls are registered only for the `logs-data-source-profile` contextual profile. https://github.com/user-attachments/assets/40adfb19-357f-46e1-9d69-fc9c0860c832 ## 👣 Next steps - [ ] #190670 - [ ] #190460 --------- Co-authored-by: Marco Antonio Ghiani <marcoantonio.ghiani@elastic.co> Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>

## 📓 Summary Closes elastic#190457 This change extracts the logic around the logs document controls/indicator from Logs Explorer and registers the same controls in Discover through the extension point added in elastic#188762. The controls are registered only for the `logs-data-source-profile` contextual profile. https://github.com/user-attachments/assets/40adfb19-357f-46e1-9d69-fc9c0860c832 ## 👣 Next steps - [ ] elastic#190670 - [ ] elastic#190460 --------- Co-authored-by: Marco Antonio Ghiani <marcoantonio.ghiani@elastic.co> Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>

[Discover] Allow to append row leading controls

2a9e7e4

jughosta self-assigned this Jul 19, 2024

jughosta added 9 commits July 19, 2024 18:03

[Discover] Hide the remaining actions under menu button

41397f1

[Discover] Restore externalControlColumns and `trailingControlColum…

b1e58b0

…ns` props

[Discover] Update comment

8e32755

[Discover] Rename prop

6e439b2

[Discover] Switch to renderControl

e430f77

[Discover] Rename

57baf91

[Discover] Switched to the new extension

1224e71

[Discover] Refactor logs explorer actions

cce6ea8

[Discover] Move the i popover

44579d3

[Discover] Remove unused

489ac2c

jughosta added 3 commits July 25, 2024 11:28

[Discover] Refactor default control columns

2897c35

[Discover] Update logs tests

69bec46

[Discover] Update logs tests

4f86540

jughosta added 3 commits July 25, 2024 16:18

[Discover] Revert to old behaviour by default

381c3be

[Discover] Update tests

55a6f3c

Merge branch 'main' into 186637-row-actions-extension

17077f6

jughosta added 2 commits July 25, 2024 18:23

[Discover] Update screenshot

f943679

[Discover] Fix types

a2d91d9

kertal mentioned this pull request Aug 1, 2024

[OneDiscover][UnifiedDataTable] Change expand document/row button #186808

Closed

[Discover] Tmp lower the screenshot test threshold

4edef40

jughosta requested a review from a team as a code owner August 1, 2024 15:57

[Discover] Update the screenshot for tests

b058278

davismcphee approved these changes Aug 2, 2024

View reviewed changes

Heenawter approved these changes Aug 2, 2024

View reviewed changes

Merge remote-tracking branch 'upstream/main' into 186637-row-actions-…

c5bb1ef

…extension # Conflicts: # src/plugins/discover/public/context_awareness/profile_providers/example_data_source_profile/profile.tsx # src/plugins/discover/public/context_awareness/types.ts

tonyghiani reviewed Aug 5, 2024

View reviewed changes

logeekal reviewed Aug 5, 2024

View reviewed changes

jughosta added 3 commits August 6, 2024 11:04

Merge branch 'main' into 186637-row-actions-extension

4849c02

[OneDiscover] Remove Actions popover from grid header

848a1a6

[OneDiscover] Clean up translations

78fd61a

tonyghiani approved these changes Aug 6, 2024

View reviewed changes

[OneDiscover] Update comment

499ab67

logeekal approved these changes Aug 7, 2024

View reviewed changes

Merge branch 'main' into 186637-row-actions-extension

e8f6fb8

jughosta enabled auto-merge (squash) August 7, 2024 12:31

jughosta merged commit 1c4b5c7 into elastic:main Aug 7, 2024
22 checks passed

kibanamachine added the v8.16.0 label Aug 7, 2024

tonyghiani mentioned this pull request Aug 19, 2024

[One Discover] Add row indicators for log documents #190676

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[OneDiscover][Extension] DataTable Row Actions #188762

[OneDiscover][Extension] DataTable Row Actions #188762

jughosta commented Jul 19, 2024 •

edited

Loading

jughosta commented Jul 24, 2024

jughosta commented Jul 25, 2024

jughosta commented Jul 25, 2024

jughosta commented Jul 25, 2024

jughosta commented Jul 25, 2024

jughosta commented Aug 1, 2024

kertal commented Aug 1, 2024

davismcphee left a comment

Heenawter left a comment •

edited

Loading

tonyghiani left a comment

jughosta commented Aug 5, 2024

logeekal left a comment •

edited

Loading

logeekal Aug 5, 2024

jughosta Aug 6, 2024

logeekal Aug 6, 2024

jughosta Aug 7, 2024

logeekal Aug 5, 2024

jughosta Aug 6, 2024

tonyghiani commented Aug 5, 2024

tonyghiani left a comment

jughosta commented Aug 6, 2024

logeekal left a comment

logeekal commented Aug 7, 2024 •

edited

Loading

kibana-ci commented Aug 7, 2024 •

edited

Loading

API count

async chunk count

References to deprecated APIs

[OneDiscover][Extension] DataTable Row Actions #188762

[OneDiscover][Extension] DataTable Row Actions #188762

Conversation

jughosta commented Jul 19, 2024 • edited Loading

Summary

Testing

Follow up for Security solution

Checklist

jughosta commented Jul 24, 2024

jughosta commented Jul 25, 2024

jughosta commented Jul 25, 2024

jughosta commented Jul 25, 2024

jughosta commented Jul 25, 2024

jughosta commented Aug 1, 2024

kertal commented Aug 1, 2024

davismcphee left a comment

Choose a reason for hiding this comment

Heenawter left a comment • edited Loading

Choose a reason for hiding this comment

tonyghiani left a comment

Choose a reason for hiding this comment

jughosta commented Aug 5, 2024

logeekal left a comment • edited Loading

Choose a reason for hiding this comment

logeekal Aug 5, 2024

Choose a reason for hiding this comment

jughosta Aug 6, 2024

Choose a reason for hiding this comment

logeekal Aug 6, 2024

Choose a reason for hiding this comment

jughosta Aug 7, 2024

Choose a reason for hiding this comment

logeekal Aug 5, 2024

Choose a reason for hiding this comment

jughosta Aug 6, 2024

Choose a reason for hiding this comment

tonyghiani commented Aug 5, 2024

tonyghiani left a comment

Choose a reason for hiding this comment

jughosta commented Aug 6, 2024

logeekal left a comment

Choose a reason for hiding this comment

logeekal commented Aug 7, 2024 • edited Loading

kibana-ci commented Aug 7, 2024 • edited Loading

💚 Build Succeeded

Metrics [docs]

Module Count

Public APIs missing comments

Async chunks

Public APIs missing exports

Page load bundle

API count

async chunk count

References to deprecated APIs

History

jughosta commented Jul 19, 2024 •

edited

Loading

Heenawter left a comment •

edited

Loading

logeekal left a comment •

edited

Loading

logeekal commented Aug 7, 2024 •

edited

Loading

kibana-ci commented Aug 7, 2024 •

edited

Loading