-
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] [RAC] Event rendered view #108644
Conversation
Pinging @elastic/security-threat-hunting (Team:Threat Hunting) |
x-pack/plugins/timelines/public/components/t_grid/event_rendered_view/selector/index.tsx
Outdated
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.
Are we able to have a default event renderer? I'm still seeing the reason statement in place of the event summary
I think the text within the dropdown to switch views can be updated (we reference 'Event renderer' in the title and then 'summary' in the description). I know this was a carry-over from an older design mockup
Suggestion: View a rendering of the event flow for each alert
@MikePaquette @paulewing Thoughts here?
Change the gutter to medium
between the additional filters and grid/event view dropdown
Looks like there's an odd width change happening on hover that is causing the scrollbars to appear/disappear. Not sure if it's related to this PR or not
@mdefazio I thought that the default event render will be the reason that's the way I understand it @paulewing ? |
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.
Fixing my comments to be a bit clearer:
- The goal for this view was to have a way to brows only the event renders without the extra bits from the data view. So by including the reason statement within this, we're going back on what we hoped to achieve here. It will also be confusing if a user switches to this explicit view and does not seen an event renderer
- Suggestion for dropdown text:
View a rendering of the event flow for each alert
@MikePaquette @paulewing Thoughts here? - Change the gutter to
medium
between the additional filters and grid/event view dropdown (didn't check if this was already completed) - An odd width change happening on hover that is causing the scrollbars to appear/disappear. Not sure if it's related to this PR or not
💛 Build succeeded, but was flaky
Test FailuresKibana Pipeline / general / Chrome X-Pack UI Functional Tests.x-pack/test/functional/apps/logstash/pipeline_create·js.logstash pipeline create new cancel button discards the pipeline and redirects to the listStandard Out
Stack Trace
Metrics [docs]Module Count
Async chunks
Page load bundle
History
To update your PR or re-run it, just comment with: |
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.
✅ Strange hover + scrollbar addition bug seems to be fixed
Seeing a few other points of concern: Not sure if the last few are related to this PR.
1/ Looks like our select all checkbox is in the same thead cell as the actions. Also then causing the action icons to ben too close the the row checkbox
2/ We should define widths for the first few columns so there's not so much space between the actions, timestamp and rule
(~140px for actions, but likely needs to be less since this includes the checkbox column)
(200px for Timestamp)
3/ Seeing a few rule rows that do not have default reason statements on them
4/ I'm seeing horizontal scrolling by just a little bit. Shouldn't this be possible to avoid? with this view (assuming the row renderers can break lines according to their container)
5/ I'm seeing a few options in the renderer that still have dragging available
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.
See list of issues based on review. Approving based on feature flag decision.
> | ||
<ContainerEuiSelectable> | ||
<EuiSelectable | ||
aria-label="Basic example" |
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.
nit: may want to change this from the eui example
() => [ | ||
{ | ||
label: gridView, | ||
key: 'gridView', |
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.
nit: we can toss gridView
and eventRenderedView
in constants
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! 💪🏾
💚 Backport successful
This backport PR will be merged automatically after passing CI. |
* wip * match design for selecting grid view * wip to integrate event rendered view * wip * integration of the event rendered * fix perPage action on Euibasic table * Add bulding block background color to EventRenderedView * styling * remove header * fix types * fix unit tests * use memo for listProps * fix styling + add feature flag * review I * fix merge * change the gutter size Co-authored-by: Pablo Neves Machado <pablo.nevesmachado@elastic.co> Co-authored-by: Angela Chuang <yi-chun.chuang@elastic.co>
* wip * match design for selecting grid view * wip to integrate event rendered view * wip * integration of the event rendered * fix perPage action on Euibasic table * Add bulding block background color to EventRenderedView * styling * remove header * fix types * fix unit tests * use memo for listProps * fix styling + add feature flag * review I * fix merge * change the gutter size Co-authored-by: Pablo Neves Machado <pablo.nevesmachado@elastic.co> Co-authored-by: Angela Chuang <yi-chun.chuang@elastic.co> Co-authored-by: Xavier Mouligneau <189600+XavierM@users.noreply.github.com> Co-authored-by: Pablo Neves Machado <pablo.nevesmachado@elastic.co> Co-authored-by: Angela Chuang <yi-chun.chuang@elastic.co>
Summary
Integrating the Event rendered view in alerts page to have a better way to view the row render
Checklist