From 7530340e520d99b252e64558cf6debb9577f6ce7 Mon Sep 17 00:00:00 2001 From: Andrew Goldstein Date: Tue, 9 Mar 2021 13:30:11 -0700 Subject: [PATCH 1/3] ## [Security Solution] Eliminates a redundant external link icon - Fixes an issue where [a redundant external link icon](https://github.com/elastic/kibana/issues/89084) was rendered next to port numbers Per the [EuiLink documentation](https://elastic.github.io/eui/#/navigation/link), it's no longer necessary to render our own icon, because `EuiLink` will automatically display one when `target="_blank"` is passed as a prop to the link. - Updates the existing link icon unit test such that it asserts a specific icon count to catch any regressions ### Before before ### After after ### Desk testing Desk tested in: - Chrome `89.0.4389.82` - Firefox `86.0` - Safari `14.0.3` --- .../public/common/components/links/index.tsx | 27 ++++++++++++------- .../port/__snapshots__/index.test.tsx.snap | 1 - .../network/components/port/index.test.tsx | 4 +-- .../public/network/components/port/index.tsx | 2 -- 4 files changed, 20 insertions(+), 14 deletions(-) diff --git a/x-pack/plugins/security_solution/public/common/components/links/index.tsx b/x-pack/plugins/security_solution/public/common/components/links/index.tsx index 8e2f57a1a597c8..ab21a39a63e21c 100644 --- a/x-pack/plugins/security_solution/public/common/components/links/index.tsx +++ b/x-pack/plugins/security_solution/public/common/components/links/index.tsx @@ -54,6 +54,13 @@ export const LinkAnchor: React.FC = ({ children, ...props }) => ( {children} ); +export const PortContainer = styled.div` + & svg { + position: relative; + top: -1px; + } +`; + // Internal Links const HostDetailsLinkComponent: React.FC<{ children?: React.ReactNode; @@ -229,15 +236,17 @@ export const PortOrServiceNameLink = React.memo<{ children?: React.ReactNode; portOrServiceName: number | string; }>(({ children, portOrServiceName }) => ( - - {children ? children : portOrServiceName} - + + + {children ? children : portOrServiceName} + + )); PortOrServiceNameLink.displayName = 'PortOrServiceNameLink'; diff --git a/x-pack/plugins/security_solution/public/network/components/port/__snapshots__/index.test.tsx.snap b/x-pack/plugins/security_solution/public/network/components/port/__snapshots__/index.test.tsx.snap index ecdaee0ab2d938..f84e858d205736 100644 --- a/x-pack/plugins/security_solution/public/network/components/port/__snapshots__/index.test.tsx.snap +++ b/x-pack/plugins/security_solution/public/network/components/port/__snapshots__/index.test.tsx.snap @@ -11,6 +11,5 @@ exports[`Port renders correctly against snapshot 1`] = ` - `; diff --git a/x-pack/plugins/security_solution/public/network/components/port/index.test.tsx b/x-pack/plugins/security_solution/public/network/components/port/index.test.tsx index 47dfffefe091c5..9c7a0833b24bb8 100644 --- a/x-pack/plugins/security_solution/public/network/components/port/index.test.tsx +++ b/x-pack/plugins/security_solution/public/network/components/port/index.test.tsx @@ -60,13 +60,13 @@ describe('Port', () => { ); }); - test('it renders an external link', () => { + test('it renders only one external link icon', () => { const wrapper = mount( ); - expect(wrapper.find('[data-test-subj="external-link-icon"]').first().exists()).toBe(true); + expect(wrapper.find('span [data-euiicon-type="popout"]').length).toBe(1); }); }); diff --git a/x-pack/plugins/security_solution/public/network/components/port/index.tsx b/x-pack/plugins/security_solution/public/network/components/port/index.tsx index 0744ca175aa381..8ee1616d4c77bc 100644 --- a/x-pack/plugins/security_solution/public/network/components/port/index.tsx +++ b/x-pack/plugins/security_solution/public/network/components/port/index.tsx @@ -9,7 +9,6 @@ import React from 'react'; import { DefaultDraggable } from '../../../common/components/draggables'; import { getEmptyValue } from '../../../common/components/empty_value'; -import { ExternalLinkIcon } from '../../../common/components/external_link_icon'; import { PortOrServiceNameLink } from '../../../common/components/links'; export const CLIENT_PORT_FIELD_NAME = 'client.port'; @@ -40,7 +39,6 @@ export const Port = React.memo<{ value={value} > - )); From 67ae143bc0f560c2eaa91a943884eaf22bd5e747 Mon Sep 17 00:00:00 2001 From: Andrew Goldstein Date: Wed, 10 Mar 2021 10:46:40 -0700 Subject: [PATCH 2/3] - pr feedback --- .../external_link_icon/index.test.tsx | 76 ------------------- .../components/external_link_icon/index.tsx | 48 ------------ .../common/components/links/index.test.tsx | 16 ++-- .../public/common/components/links/index.tsx | 2 - .../certificate_fingerprint/index.tsx | 2 - .../components/ja3_fingerprint/index.tsx | 2 - .../row_renderers_browser/catalog/index.tsx | 2 - .../body/renderers/suricata/suricata_refs.tsx | 2 - 8 files changed, 9 insertions(+), 141 deletions(-) delete mode 100644 x-pack/plugins/security_solution/public/common/components/external_link_icon/index.test.tsx delete mode 100644 x-pack/plugins/security_solution/public/common/components/external_link_icon/index.tsx diff --git a/x-pack/plugins/security_solution/public/common/components/external_link_icon/index.test.tsx b/x-pack/plugins/security_solution/public/common/components/external_link_icon/index.test.tsx deleted file mode 100644 index 65da5423d19e89..00000000000000 --- a/x-pack/plugins/security_solution/public/common/components/external_link_icon/index.test.tsx +++ /dev/null @@ -1,76 +0,0 @@ -/* - * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one - * or more contributor license agreements. Licensed under the Elastic License - * 2.0; you may not use this file except in compliance with the Elastic License - * 2.0. - */ - -import { mount } from 'enzyme'; -import React from 'react'; - -import { TestProviders } from '../../mock'; - -import { ExternalLinkIcon } from '.'; - -describe('Duration', () => { - test('it renders expected icon type when the leftMargin prop is not specified', () => { - const wrapper = mount( - - - - ); - - expect(wrapper.find('[data-test-subj="external-link-icon"]').first().props().type).toEqual( - 'popout' - ); - }); - - test('it renders expected icon type when the leftMargin prop is true', () => { - const wrapper = mount( - - - - ); - - expect(wrapper.find('[data-test-subj="external-link-icon"]').first().props().type).toEqual( - 'popout' - ); - }); - - test('it applies a margin-left style when the leftMargin prop is true', () => { - const wrapper = mount( - - - - ); - - expect(wrapper.find('[data-test-subj="external-link-icon"]').first()).toHaveStyleRule( - 'margin-left', - '5px' - ); - }); - - test('it does NOT apply a margin-left style when the leftMargin prop is false', () => { - const wrapper = mount( - - - - ); - - expect(wrapper.find('[data-test-subj="external-link-icon"]').first()).not.toHaveStyleRule( - 'margin-left' - ); - }); - - test('it renders expected icon type when the leftMargin prop is false', () => { - const wrapper = mount( - - - - ); - - expect(wrapper.find('[data-test-subj="external-link-icon"]').first().props().type).toEqual( - 'popout' - ); - }); -}); diff --git a/x-pack/plugins/security_solution/public/common/components/external_link_icon/index.tsx b/x-pack/plugins/security_solution/public/common/components/external_link_icon/index.tsx deleted file mode 100644 index 825ee8a33a8a5a..00000000000000 --- a/x-pack/plugins/security_solution/public/common/components/external_link_icon/index.tsx +++ /dev/null @@ -1,48 +0,0 @@ -/* - * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one - * or more contributor license agreements. Licensed under the Elastic License - * 2.0; you may not use this file except in compliance with the Elastic License - * 2.0. - */ - -import { EuiIcon } from '@elastic/eui'; -import React from 'react'; -import styled from 'styled-components'; - -const LinkIcon = styled(EuiIcon)` - position: relative; - top: -2px; -`; - -LinkIcon.displayName = 'LinkIcon'; - -const LinkIconWithMargin = styled(LinkIcon)` - margin-left: 5px; -`; - -LinkIconWithMargin.displayName = 'LinkIconWithMargin'; - -const color = 'subdued'; -const iconSize = 's'; -const iconType = 'popout'; - -/** - * Renders an icon that indicates following the hyperlink will navigate to - * content external to the app - */ -export const ExternalLinkIcon = React.memo<{ - leftMargin?: boolean; -}>(({ leftMargin = true }) => - leftMargin ? ( - - ) : ( - - ) -); - -ExternalLinkIcon.displayName = 'ExternalLinkIcon'; diff --git a/x-pack/plugins/security_solution/public/common/components/links/index.test.tsx b/x-pack/plugins/security_solution/public/common/components/links/index.test.tsx index 864e55b3e4a453..cd19eb5a27d7b1 100644 --- a/x-pack/plugins/security_solution/public/common/components/links/index.test.tsx +++ b/x-pack/plugins/security_solution/public/common/components/links/index.test.tsx @@ -5,7 +5,7 @@ * 2.0. */ -import { mount, shallow, ShallowWrapper } from 'enzyme'; +import { mount, shallow, ReactWrapper, ShallowWrapper } from 'enzyme'; import React from 'react'; import { removeExternalLinkText } from '../../../../common/test_utils'; import { mountWithIntl } from '@kbn/test/jest'; @@ -121,11 +121,11 @@ describe('Custom Links', () => { describe('External Link', () => { const mockLink = 'https://www.virustotal.com/gui/search/'; const mockLinkName = 'Link'; - let wrapper: ShallowWrapper; + let wrapper: ReactWrapper | ShallowWrapper; describe('render', () => { beforeAll(() => { - wrapper = shallow( + wrapper = mount( {mockLinkName} @@ -137,11 +137,13 @@ describe('Custom Links', () => { }); test('it renders ExternalLinkIcon', () => { - expect(wrapper.find('[data-test-subj="externalLinkIcon"]').exists()).toBeTruthy(); + expect(wrapper.find('span [data-euiicon-type="popout"]').length).toBe(1); }); test('it renders correct url', () => { - expect(wrapper.find('[data-test-subj="externalLink"]').prop('href')).toEqual(mockLink); + expect(wrapper.find('[data-test-subj="externalLink"]').first().prop('href')).toEqual( + mockLink + ); }); test('it renders comma if id is given', () => { @@ -435,14 +437,14 @@ describe('Custom Links', () => { test('it renders correct number of external icons by default', () => { const wrapper = mountWithIntl(); - expect(wrapper.find('[data-test-subj="externalLinkIcon"]')).toHaveLength(5); + expect(wrapper.find('span [data-euiicon-type="popout"]')).toHaveLength(5); }); test('it renders correct number of external icons', () => { const wrapper = mountWithIntl( ); - expect(wrapper.find('[data-test-subj="externalLinkIcon"]')).toHaveLength(1); + expect(wrapper.find('span [data-euiicon-type="popout"]')).toHaveLength(1); }); }); }); diff --git a/x-pack/plugins/security_solution/public/common/components/links/index.tsx b/x-pack/plugins/security_solution/public/common/components/links/index.tsx index ab21a39a63e21c..e7669fbac987ee 100644 --- a/x-pack/plugins/security_solution/public/common/components/links/index.tsx +++ b/x-pack/plugins/security_solution/public/common/components/links/index.tsx @@ -39,7 +39,6 @@ import { } from '../../../../common/search_strategy/security_solution/network'; import { useUiSetting$, useKibana } from '../../lib/kibana'; import { isUrlInvalid } from '../../utils/validators'; -import { ExternalLinkIcon } from '../external_link_icon'; import * as i18n from './translations'; import { SecurityPageName } from '../../../app/types'; @@ -121,7 +120,6 @@ export const ExternalLink = React.memo<{ {children} - {!isNil(idx) && idx < lastIndexToShow && } diff --git a/x-pack/plugins/security_solution/public/timelines/components/certificate_fingerprint/index.tsx b/x-pack/plugins/security_solution/public/timelines/components/certificate_fingerprint/index.tsx index d850204284bd0d..29775067478a59 100644 --- a/x-pack/plugins/security_solution/public/timelines/components/certificate_fingerprint/index.tsx +++ b/x-pack/plugins/security_solution/public/timelines/components/certificate_fingerprint/index.tsx @@ -10,7 +10,6 @@ import React from 'react'; import styled from 'styled-components'; import { DraggableBadge } from '../../../common/components/draggables'; -import { ExternalLinkIcon } from '../../../common/components/external_link_icon'; import { CertificateFingerprintLink } from '../../../common/components/links'; import * as i18n from './translations'; @@ -61,7 +60,6 @@ export const CertificateFingerprint = React.memo<{ {certificateType === 'client' ? i18n.CLIENT_CERT : i18n.SERVER_CERT} - ); }); diff --git a/x-pack/plugins/security_solution/public/timelines/components/ja3_fingerprint/index.tsx b/x-pack/plugins/security_solution/public/timelines/components/ja3_fingerprint/index.tsx index df8c68df483c53..d73130417566f3 100644 --- a/x-pack/plugins/security_solution/public/timelines/components/ja3_fingerprint/index.tsx +++ b/x-pack/plugins/security_solution/public/timelines/components/ja3_fingerprint/index.tsx @@ -9,7 +9,6 @@ import React from 'react'; import styled from 'styled-components'; import { DraggableBadge } from '../../../common/components/draggables'; -import { ExternalLinkIcon } from '../../../common/components/external_link_icon'; import { Ja3FingerprintLink } from '../../../common/components/links'; import * as i18n from './translations'; @@ -45,7 +44,6 @@ export const Ja3Fingerprint = React.memo<{ {i18n.JA3_FINGERPRINT_LABEL} - )); diff --git a/x-pack/plugins/security_solution/public/timelines/components/row_renderers_browser/catalog/index.tsx b/x-pack/plugins/security_solution/public/timelines/components/row_renderers_browser/catalog/index.tsx index 65974a10c49c21..283a239acad24b 100644 --- a/x-pack/plugins/security_solution/public/timelines/components/row_renderers_browser/catalog/index.tsx +++ b/x-pack/plugins/security_solution/public/timelines/components/row_renderers_browser/catalog/index.tsx @@ -7,7 +7,6 @@ import { EuiLink } from '@elastic/eui'; import React from 'react'; -import { ExternalLinkIcon } from '../../../../common/components/external_link_icon'; import { RowRendererId } from '../../../../../common/types/timeline'; import { @@ -37,7 +36,6 @@ const Link = ({ children, url }: { children: React.ReactNode; url: string }) => data-test-subj="externalLink" > {children} - ); diff --git a/x-pack/plugins/security_solution/public/timelines/components/timeline/body/renderers/suricata/suricata_refs.tsx b/x-pack/plugins/security_solution/public/timelines/components/timeline/body/renderers/suricata/suricata_refs.tsx index 0bbd86479c2268..96e6b916a3e11e 100644 --- a/x-pack/plugins/security_solution/public/timelines/components/timeline/body/renderers/suricata/suricata_refs.tsx +++ b/x-pack/plugins/security_solution/public/timelines/components/timeline/body/renderers/suricata/suricata_refs.tsx @@ -9,7 +9,6 @@ import { EuiFlexGroup, EuiFlexItem, EuiLink } from '@elastic/eui'; import React from 'react'; import styled from 'styled-components'; -import { ExternalLinkIcon } from '../../../../../../common/components/external_link_icon'; import { getLinksFromSignature } from './suricata_links'; const LinkEuiFlexItem = styled(EuiFlexItem)` @@ -27,7 +26,6 @@ export const SuricataRefs = React.memo<{ signatureId: number }>(({ signatureId } {link} - ))} From cf824f63964443a128e2b043532c5714032bb88c Mon Sep 17 00:00:00 2001 From: Andrew Goldstein Date: Wed, 10 Mar 2021 12:50:46 -0700 Subject: [PATCH 3/3] - render the comma after the icon --- .../public/common/components/links/index.tsx | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/x-pack/plugins/security_solution/public/common/components/links/index.tsx b/x-pack/plugins/security_solution/public/common/components/links/index.tsx index e7669fbac987ee..a02cc8bf76bcc0 100644 --- a/x-pack/plugins/security_solution/public/common/components/links/index.tsx +++ b/x-pack/plugins/security_solution/public/common/components/links/index.tsx @@ -118,10 +118,12 @@ export const ExternalLink = React.memo<{ const inAllowlist = allowedUrlSchemes.some((scheme) => url.indexOf(scheme) === 0); return url && inAllowlist && !isUrlInvalid(url) && children ? ( - - {children} + <> + + {children} + {!isNil(idx) && idx < lastIndexToShow && } - + ) : null; }