Skip to content

Commit

Permalink
[Security Solution] Eliminates a redundant external link icon (#94194)
Browse files Browse the repository at this point in the history
## [Security Solution] Eliminates a redundant external link icon

- Fixes an issue where [a redundant external link icon](#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

<img width="1673" alt="before" src="https://user-images.githubusercontent.com/4459398/110530119-4cd0ac00-80d7-11eb-9d54-5d6656491e69.png">

### After

<img width="1677" alt="after" src="https://user-images.githubusercontent.com/4459398/110530165-5c4ff500-80d7-11eb-99a3-68741fab9218.png">

### Desk testing

Desk tested in:

- Chrome `89.0.4389.82`
- Firefox `86.0`
- Safari `14.0.3`
  • Loading branch information
andrew-goldstein authored Mar 11, 2021
1 parent 9aeb9f4 commit 14c32cb
Show file tree
Hide file tree
Showing 11 changed files with 34 additions and 158 deletions.

This file was deleted.

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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(
<ExternalLink url={mockLink} idx={0} allItemsLimit={5} overflowIndexStart={5}>
{mockLinkName}
</ExternalLink>
Expand All @@ -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', () => {
Expand Down Expand Up @@ -435,14 +437,14 @@ describe('Custom Links', () => {

test('it renders correct number of external icons by default', () => {
const wrapper = mountWithIntl(<ReputationLink domain={'192.0.2.0'} />);
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(
<ReputationLink domain={'192.0.2.0'} overflowIndexStart={1} />
);
expect(wrapper.find('[data-test-subj="externalLinkIcon"]')).toHaveLength(1);
expect(wrapper.find('span [data-euiicon-type="popout"]')).toHaveLength(1);
});
});
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand All @@ -54,6 +53,13 @@ export const LinkAnchor: React.FC<EuiLinkProps> = ({ children, ...props }) => (
<EuiLink {...props}>{children}</EuiLink>
);

export const PortContainer = styled.div`
& svg {
position: relative;
top: -1px;
}
`;

// Internal Links
const HostDetailsLinkComponent: React.FC<{
children?: React.ReactNode;
Expand Down Expand Up @@ -112,11 +118,12 @@ export const ExternalLink = React.memo<{
const inAllowlist = allowedUrlSchemes.some((scheme) => url.indexOf(scheme) === 0);
return url && inAllowlist && !isUrlInvalid(url) && children ? (
<EuiToolTip content={url} position="top" data-test-subj="externalLinkTooltip">
<EuiLink href={url} target="_blank" rel="noopener" data-test-subj="externalLink">
{children}
<ExternalLinkIcon data-test-subj="externalLinkIcon" />
<>
<EuiLink href={url} target="_blank" rel="noopener" data-test-subj="externalLink">
{children}
</EuiLink>
{!isNil(idx) && idx < lastIndexToShow && <Comma data-test-subj="externalLinkComma" />}
</EuiLink>
</>
</EuiToolTip>
) : null;
}
Expand Down Expand Up @@ -229,15 +236,17 @@ export const PortOrServiceNameLink = React.memo<{
children?: React.ReactNode;
portOrServiceName: number | string;
}>(({ children, portOrServiceName }) => (
<EuiLink
data-test-subj="port-or-service-name-link"
href={`https://www.iana.org/assignments/service-names-port-numbers/service-names-port-numbers.xhtml?search=${encodeURIComponent(
String(portOrServiceName)
)}`}
target="_blank"
>
{children ? children : portOrServiceName}
</EuiLink>
<PortContainer>
<EuiLink
data-test-subj="port-or-service-name-link"
href={`https://www.iana.org/assignments/service-names-port-numbers/service-names-port-numbers.xhtml?search=${encodeURIComponent(
String(portOrServiceName)
)}`}
target="_blank"
>
{children ? children : portOrServiceName}
</EuiLink>
</PortContainer>
));

PortOrServiceNameLink.displayName = 'PortOrServiceNameLink';
Expand Down

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Original file line number Diff line number Diff line change
Expand Up @@ -60,13 +60,13 @@ describe('Port', () => {
);
});

test('it renders an external link', () => {
test('it renders only one external link icon', () => {
const wrapper = mount(
<TestProviders>
<Port contextId="test" eventId="abcd" fieldName="destination.port" value="443" />
</TestProviders>
);

expect(wrapper.find('[data-test-subj="external-link-icon"]').first().exists()).toBe(true);
expect(wrapper.find('span [data-euiicon-type="popout"]').length).toBe(1);
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -40,7 +39,6 @@ export const Port = React.memo<{
value={value}
>
<PortOrServiceNameLink portOrServiceName={value || getEmptyValue()} />
<ExternalLinkIcon />
</DefaultDraggable>
));

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -61,7 +60,6 @@ export const CertificateFingerprint = React.memo<{
{certificateType === 'client' ? i18n.CLIENT_CERT : i18n.SERVER_CERT}
</FingerprintLabel>
<CertificateFingerprintLink certificateFingerprint={value || ''} />
<ExternalLinkIcon />
</DraggableBadge>
);
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -45,7 +44,6 @@ export const Ja3Fingerprint = React.memo<{
{i18n.JA3_FINGERPRINT_LABEL}
</Ja3FingerprintLabel>
<Ja3FingerprintLink data-test-subj="ja3-hash-link" ja3Fingerprint={value || ''} />
<ExternalLinkIcon />
</DraggableBadge>
));

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -37,7 +36,6 @@ const Link = ({ children, url }: { children: React.ReactNode; url: string }) =>
data-test-subj="externalLink"
>
{children}
<ExternalLinkIcon data-test-subj="externalLinkIcon" />
</EuiLink>
);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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)`
Expand All @@ -27,7 +26,6 @@ export const SuricataRefs = React.memo<{ signatureId: number }>(({ signatureId }
<EuiLink href={link} color="subdued" target="_blank">
{link}
</EuiLink>
<ExternalLinkIcon />
</LinkEuiFlexItem>
))}
</EuiFlexGroup>
Expand Down

0 comments on commit 14c32cb

Please sign in to comment.