-
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
[SIEM] Adds custom tooltip to map for dragging fields to timeline #46879
Conversation
…within an EuiPopover
Pinging @elastic/siem |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
…rting other popover draggables, feature cycling, and add filters options
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
expect(toJson(wrapper)).toMatchSnapshot(); | ||
}); | ||
|
||
test('previousButton is functioning and nextButton is not when featureIndex is 0', () => { |
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.
should this read something like previousButton is disabled and nextButton is functioning when featureIndex is 0
?
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.
consider structuring the tests in this file such that there is one expect
per test
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.
@@ -127,7 +131,7 @@ const ProviderContainer = styled.div<{ isDragging: boolean }>` | |||
${isDragging && | |||
` | |||
& { | |||
z-index: ${theme.eui.euiZLevel9} !important; | |||
z-index: 9999 !important; |
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.
Any thoughts on how to better handle this situation? In this instance, the map tooltip EuiPopover
is managed in the maps code, so we can't pass in our own z-index
. When you don't specify your own z-index
, the tooltip EuiPopover
does some calculation to ensure it's under any flyout that would appear. This isn't necessary in this implementation as we have an outside click detector that'll close the popover.
The map tooltip EuiPopover
ends up with the following z-index
:
.euiPopover__panel.euiPopover__panel-isOpen {
z-index: 9900 !important;
}
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
@@ -7,6 +7,32 @@ | |||
import uuid from 'uuid'; | |||
import { IndexPatternMapping } from './types'; | |||
|
|||
// Update source/destination field mappings to modify what fields will be returned to map tooltip | |||
const sourceFieldMappings: Record<string, string> = { | |||
'host.name': 'Host', |
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 i18n required here (for the values Host
, Source IP
, etc, and in the destinationFieldMappings
const 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.
Nice catch! And that's why you should always write your i18n the same time you write any user facing string, ha! 🙂
import { npStart } from 'ui/new_platform'; | ||
import { OutPortal, PortalNode } from 'react-reverse-portal'; |
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.
Just noting (per a conversation with @spong ), that the re-parenting provided by react-reverse-portal
here is required here because when MapToolTip
is rendered in the embeddables component tree (the default behavior), it doesn't have access to the Redux store, which is required to register and un-register the draggable.
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.
That's correct. Adding the following comment to embeddable_map
to make it clear for anyone meandering through this code at a later date. Thanks for specifying!
// This portalNode provided by react-reverse-portal allows us re-parent the MapToolTip within our
// own component tree instead of the embeddables (default). This is necessary to have access to
// the Redux store, theme provider, etc, which is required to register and un-register the draggable
// Search InPortal/OutPortal for implementation touch points
const portalNode = React.useMemo(() => createPortalNode(), []);
@@ -53,6 +55,8 @@ export const EmbeddedMap = React.memo<EmbeddedMapProps>( | |||
const [loadingKibanaIndexPatterns, kibanaIndexPatterns] = useIndexPatterns(); | |||
const [siemDefaultIndices] = useKibanaUiSetting(DEFAULT_INDEX_KEY); | |||
|
|||
const portalNode = React.useMemo(() => createPortalNode(), []); |
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.
createPortalNode()
feels like an effect, which makes me wonder if useEffect
is technically more appropriate than useMemo
here, due to following statement in the useMemo reference:
Remember that the function passed to useMemo runs during rendering. Don’t do anything there that you wouldn’t normally do while rendering. For example, side effects belong in useEffect, not useMemo.
What do you think?
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 was following suit from the example docs, but as you pointed out later in the docs it certainly sounds like an effect. Feels like a grey area -- let's keep an eye on any wonky behavior, but maintain it as described in the example.
Thanks for calling attention to this 🙂
<EuiFlexItem> | ||
<FlowBadge color="hollow"> | ||
<EuiFlexGroupStyled direction="column"> | ||
<EuiFlexItem grow={false}>{'Source'}</EuiFlexItem> |
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.
does Source
require i18n?
<EuiFlexItem> | ||
<FlowBadge color="hollow"> | ||
<EuiFlexGroupStyled> | ||
<EuiFlexItem grow={false}>{'Destination'}</EuiFlexItem> |
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.
i18n?
layerId: features[featureIndex].layerId, | ||
featureId: features[featureIndex].id, | ||
}), | ||
getLayerName(features[featureIndex].layerId), |
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.
optional nit: replace all the repeated features[featureIndex].layerId
and features[featureIndex].id
with
const layerId = features[featureIndex].layerId;
const featureId = features[featureIndex].id;
and then just refer to layerId
, featureId
setFeaturePropsFilters(featurePropsESFilters); | ||
setFeatureGeometry(featureGeo); | ||
setLayerName(layerNameString); | ||
setIsLoading(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.
Will this cleanup code (resetting loading indicators) execute if any of the promises in
const featurePropsPromises = await Promise.all(...
reject?
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 just dug a bit into this and it would've resulted in an unhandled Promise rejection and the cleanup not running. One option would be to reject, but would need to do that for each promise, so I opted for try/catch/finally and adding an error state. Thanks for calling attention to this!
💚 Build Succeeded |
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 @spong for tenaciously overcoming all the technical challenges required to achieve drag and drop from portal content, and for making it possible to re-use the techniques you established going forward with other embedded content and portals! 🚀
The new feature that summarizes the data transferred between source and destination when clicking a line was useful while desk testing for quickly determining whether or not some of the more... unusual hosts talking to well-known hosts were just a blip on the radar (during the selected date range).
Tested locally in Chrome, Firefox, and Safari.
🔴 LGTM 🔵
💚 Build Succeeded |
When testing in IE11 ran into #47609. Will use that issue for tracking and resolve after FF for 7.5. |
…astic#46879) Resolves elastic#46301, by adding a custom tooltip for the map that enables dragging to the timeline. * Adds new portal pattern to enable DnD from outside the main react component tree * Adds `<DraggablePortalContext>` component to enable DnD from within an `EuiPopover` * Just wrap `EuiPopover` contents in `<DraggablePortalContext.Provider value={true}></...>` and all child `DefaultDraggable`'s will now function correctly * Displays netflow renderer within tooltip for line features, w/ draggable src/dest.bytes * Displays detailed description list within tooltip for point features. Fields include: * host.name * source/destination.ip * source/destination.domain * source/destination.geo.country_iso_code * source/destination.as.organization.name * Retains ability to add filter to KQL bar ![map_custom_tooltips](https://user-images.githubusercontent.com/2946766/66288691-64c74f00-e897-11e9-9845-54e8c9b9c5ab.gif) Use ~~strikethroughs~~ to remove checklist items you don't feel are applicable to this PR. - [x] This was checked for cross-browser compatibility, [including a check against IE11](https://github.com/elastic/kibana/blob/master/CONTRIBUTING.md#cross-browser-compatibility) - [x] Any text added follows [EUI's writing guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses sentence case text and includes [i18n support](https://github.com/elastic/kibana/blob/master/packages/kbn-i18n/README.md) - [ ] ~[Documentation](https://github.com/elastic/kibana/blob/master/CONTRIBUTING.md#writing-documentation) was added for features that require explanation or tutorials~ - [x] [Unit or functional tests](https://github.com/elastic/kibana/blob/master/CONTRIBUTING.md#cross-browser-compatibility) were updated or added to match the most common scenarios - [ ] ~This was checked for [keyboard-only and screenreader accessibility](https://developer.mozilla.org/en-US/docs/Learn/Tools_and_testing/Cross_browser_testing/Accessibility#Accessibility_testing_checklist)~ - [ ] ~This was checked for breaking API changes and was [labeled appropriately](https://github.com/elastic/kibana/blob/master/CONTRIBUTING.md#release-notes-process)~ - [x] This includes a feature addition or change that requires a release note and was [labeled appropriately](https://github.com/elastic/kibana/blob/master/CONTRIBUTING.md#release-notes-process)
…astic#46879) ## Summary Resolves elastic#46301, by adding a custom tooltip for the map that enables dragging to the timeline. ##### Features: * Adds new portal pattern to enable DnD from outside the main react component tree * Adds `<DraggablePortalContext>` component to enable DnD from within an `EuiPopover` * Just wrap `EuiPopover` contents in `<DraggablePortalContext.Provider value={true}></...>` and all child `DefaultDraggable`'s will now function correctly * Displays netflow renderer within tooltip for line features, w/ draggable src/dest.bytes * Displays detailed description list within tooltip for point features. Fields include: * host.name * source/destination.ip * source/destination.domain * source/destination.geo.country_iso_code * source/destination.as.organization.name * Retains ability to add filter to KQL bar ![map_custom_tooltips](https://user-images.githubusercontent.com/2946766/66288691-64c74f00-e897-11e9-9845-54e8c9b9c5ab.gif) ### Checklist Use ~~strikethroughs~~ to remove checklist items you don't feel are applicable to this PR. - [x] This was checked for cross-browser compatibility, [including a check against IE11](https://github.com/elastic/kibana/blob/master/CONTRIBUTING.md#cross-browser-compatibility) - [x] Any text added follows [EUI's writing guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses sentence case text and includes [i18n support](https://github.com/elastic/kibana/blob/master/packages/kbn-i18n/README.md) - [ ] ~[Documentation](https://github.com/elastic/kibana/blob/master/CONTRIBUTING.md#writing-documentation) was added for features that require explanation or tutorials~ - [x] [Unit or functional tests](https://github.com/elastic/kibana/blob/master/CONTRIBUTING.md#cross-browser-compatibility) were updated or added to match the most common scenarios - [ ] ~This was checked for [keyboard-only and screenreader accessibility](https://developer.mozilla.org/en-US/docs/Learn/Tools_and_testing/Cross_browser_testing/Accessibility#Accessibility_testing_checklist)~ ### For maintainers - [ ] ~This was checked for breaking API changes and was [labeled appropriately](https://github.com/elastic/kibana/blob/master/CONTRIBUTING.md#release-notes-process)~ - [x] This includes a feature addition or change that requires a release note and was [labeled appropriately](https://github.com/elastic/kibana/blob/master/CONTRIBUTING.md#release-notes-process)
…6879) (#48357) ## Summary Resolves #46301, by adding a custom tooltip for the map that enables dragging to the timeline. ##### Features: * Adds new portal pattern to enable DnD from outside the main react component tree * Adds `<DraggablePortalContext>` component to enable DnD from within an `EuiPopover` * Just wrap `EuiPopover` contents in `<DraggablePortalContext.Provider value={true}></...>` and all child `DefaultDraggable`'s will now function correctly * Displays netflow renderer within tooltip for line features, w/ draggable src/dest.bytes * Displays detailed description list within tooltip for point features. Fields include: * host.name * source/destination.ip * source/destination.domain * source/destination.geo.country_iso_code * source/destination.as.organization.name * Retains ability to add filter to KQL bar ![map_custom_tooltips](https://user-images.githubusercontent.com/2946766/66288691-64c74f00-e897-11e9-9845-54e8c9b9c5ab.gif) ### Checklist Use ~~strikethroughs~~ to remove checklist items you don't feel are applicable to this PR. - [x] This was checked for cross-browser compatibility, [including a check against IE11](https://github.com/elastic/kibana/blob/master/CONTRIBUTING.md#cross-browser-compatibility) - [x] Any text added follows [EUI's writing guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses sentence case text and includes [i18n support](https://github.com/elastic/kibana/blob/master/packages/kbn-i18n/README.md) - [ ] ~[Documentation](https://github.com/elastic/kibana/blob/master/CONTRIBUTING.md#writing-documentation) was added for features that require explanation or tutorials~ - [x] [Unit or functional tests](https://github.com/elastic/kibana/blob/master/CONTRIBUTING.md#cross-browser-compatibility) were updated or added to match the most common scenarios - [ ] ~This was checked for [keyboard-only and screenreader accessibility](https://developer.mozilla.org/en-US/docs/Learn/Tools_and_testing/Cross_browser_testing/Accessibility#Accessibility_testing_checklist)~ ### For maintainers - [ ] ~This was checked for breaking API changes and was [labeled appropriately](https://github.com/elastic/kibana/blob/master/CONTRIBUTING.md#release-notes-process)~ - [x] This includes a feature addition or change that requires a release note and was [labeled appropriately](https://github.com/elastic/kibana/blob/master/CONTRIBUTING.md#release-notes-process)
Summary
Resolves #46301, by adding a custom tooltip for the map that enables dragging to the timeline.
Features:
<DraggablePortalContext>
component to enable DnD from within anEuiPopover
EuiPopover
contents in<DraggablePortalContext.Provider value={true}></...>
and all childDefaultDraggable
's will now function correctlyChecklist
Use
strikethroughsto remove checklist items you don't feel are applicable to this PR.Documentation was added for features that require explanation or tutorialsThis was checked for keyboard-only and screenreader accessibilityFor maintainers
This was checked for breaking API changes and was labeled appropriately