-
Notifications
You must be signed in to change notification settings - Fork 8k
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
[Osquery] Add to timeline button #128596
[Osquery] Add to timeline button #128596
Conversation
Pinging @elastic/security-asset-management (Team:Asset Management) |
@elasticmachine merge upstream |
}, | ||
}; | ||
|
||
return getAddToTimelineButton({ |
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 have at least a unit test to check the flyout is rendered correctly according to the props and then check if add to timeline was executed with the right options.
@elasticmachine merge upstream |
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.
Security solution changes LGTM
@elasticmachine merge upstream |
onClose={onClose} | ||
aria-labelledby="flyoutTitle" | ||
// eslint-disable-next-line react-perf/jsx-no-new-object-as-prop | ||
maskProps={{ style: 'z-index: 6000' }} // For an edge case to display above the alerts flyout |
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.
So I see below that this flyout has a form in it, did you verify that all the select dropdowns etc that render in this form, namely EuiSuperSelect options and anything that renders itself via a portal (not sure of everything) render correctly? I noticed recently that any element that renders itself in a portal and has a parent that is also in a portal, when it tries to determine it's own stacking context cannot have a z-index higher than 2000 or something. https://codesandbox.io/s/crimson-voice-zq510u?file=/demo.js for a demo. Mentioned this to eui but they didn't seem to think it's a bug, personally I do, https://github.com/elastic/eui/blob/main/src/services/popover/popover_positioning.ts#L743 for an element in a portal will not do what it should.
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, I don't know yet, but will take a look at this and get back to you. Big thanks! :)
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.
Thanks, I double checked and it seems to be working just fine. Selects too :)
The only thing is the Global NavBar that doesnt really work when any of the additional flyouts is opened.
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.
So we checked one more time, and apparently it works fine on Alerts, but messes up one of the flyouts in Osquery itself.
Thanks for pointing this out :)
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.
the issue is fixed here #130944
@elasticmachine merge upstream |
dataProvider: providerA, | ||
field: value, | ||
ownFocus: false, | ||
...(payload.isIcon ? { showTooltip: true } : { Component: TimelineComponent }), |
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.
is this ever called with isIcon false?
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.
No, never. It's either true
or undefined
|
||
const handleAddToTimeline = useCallback( | ||
(payload: { query: [string, string]; isIcon?: true }) => { | ||
const [field, value] = payload.query; |
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: could change this to const { query: [field, value], isIcon } = payload
so it's more clear that isIcon is used below
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.
👍
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.
unless i'm missing something not sure if you need to pass isIcon around as it always seems true, but otherwise lgtm 👍
Thanks @kqualters-elastic :) |
@elasticmachine merge upstream |
merge conflict between base and head |
# Conflicts: # x-pack/plugins/osquery/cypress/integration/all/live_query.spec.ts # x-pack/plugins/osquery/public/live_queries/form/index.tsx # x-pack/plugins/osquery/public/live_queries/index.tsx # x-pack/plugins/osquery/public/results/results_table.tsx # x-pack/plugins/osquery/public/routes/saved_queries/edit/tabs.tsx # x-pack/plugins/osquery/public/shared_components/osquery_action/index.tsx # x-pack/plugins/security_solution/public/detections/components/osquery/osquery_flyout.tsx
@elasticmachine merge upstream |
@elasticmachine merge upstream |
@elasticmachine merge upstream |
💚 Build SucceededMetrics [docs]Module Count
Async chunks
History
To update your PR or re-run it, just comment with: cc @tomsonpl |
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.
Asset management LGTM
Summary
Add possibility to quickly add osquery action to the timeline.
Also - remove the go back button from the Osquery Flyout