-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[Security Solution] integrate CellActions in Events/Alerts DataTables #149934
[Security Solution] integrate CellActions in Events/Alerts DataTables #149934
Conversation
…ithub.com/semd/kibana into 145666_cell_actions_datatable_integration
…_datatable_integration
Pinging @elastic/security-threat-hunting (Team:Threat Hunting) |
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.
Hey Sergi, Good work!
useDataGridColumnsCellActions
usage looks simple and clear. 👏
I noticed a minor issue on show_top_n
could you take a look?
x-pack/plugins/security_solution/public/common/components/data_table/index.tsx
Show resolved
Hide resolved
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.
LGTM
@elasticmachine merge upstream |
visibleCellActions: 3, | ||
} | ||
: {}), | ||
cellActions: columnsCellActions[columnIndex] ?? [], |
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.
This is the beauty - how simpler it is now!
@@ -569,17 +566,15 @@ const StatefulEventsViewerComponent: React.FC<EventsViewerProps & PropsFromRedux | |||
unitCountText={unitCountText} | |||
browserFields={browserFields} | |||
data={nonDeletedEvents} | |||
disabledCellActions={FIELDS_WITHOUT_CELL_ACTIONS} | |||
disableCellActions={disableCellActions} |
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 we should also remove file x-pack/plugins/security_solution/public/common/lib/cell_actions/constants.ts
because its FIELDS_WITHOUT_CELL_ACTIONS
is not in use any more
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.
LGTM! Great work!
I found the issue for flyout showTopN
functionality not displayed, but this is happening in the main as well, so not related to the PR.
@elasticmachine merge upstream |
@elasticmachine merge upstream |
…ithub.com/semd/kibana into 145666_cell_actions_datatable_integration
💛 Build succeeded, but was flaky
Failed CI StepsTest Failures
Metrics [docs]Module Count
Public APIs missing comments
Async chunks
History
To update your PR or re-run it, just comment with: cc @semd |
issue: #145666
Summary
Migrate DataTable component, used to render the alerts/events data grids in different pages: Alerts, Explore pages (Hosts, Users, Network), and also Rule preview. In summary, all data grids but Timeline.
The integration won't modify any action or change any functionality, everything is supposed to keep working the same way from the User's perspective. But it has a minor UI update in the actions tooltip layout:
old

new

This change was needed since the custom tooltip (old one) was adding too many Security-related customizations to the generic EuiDataGrid's columnCellActions, and also it had a lot of "action-specific" complexity, which is against the idea of having an action-agnostic generic component, to begin with.
So, in order to unify the CellActions user experience (icon, text, layout...), we needed to use a more uniformed component, that will render all the actions without any special case.