Added external prop to EuiLink #2442

andreadelrio · 2019-10-16T17:53:54Z

Summary

This PR adds external as an opt-in prop to EuiLink. Setting the prop to true adds the popout icon next to the link. Covers #2079

Checklist

- [ ] Checked in dark mode
- [ ] Checked in mobile

Checked in IE11 and Firefox
Props have proper autodocs
Added documentation examples
Added or updated jest tests
~~- [ ] Checked for breaking changes and labeled appropriately~~
~~- [ ] Checked for accessibility including keyboard-only and screenreader modes~~
A changelog entry exists and is marked appropriately

[external-link] interface and props table fix

cchaos

Thanks for taking this one on! Just a couple comments.

src-docs/src/views/link/link.js

src/components/link/_link.scss

cchaos · 2019-10-16T18:29:13Z

src/components/link/link.tsx

@@ -111,6 +118,7 @@ const EuiLink = forwardRef<HTMLAnchorElement | HTMLButtonElement, EuiLinkProps>(
        ref={ref as React.Ref<HTMLAnchorElement>}
        {...anchorProps as EuiLinkAnchorProps}>
        {children}
+        {external ? <EuiIcon type="popout" /> : undefined}


You should probably pass an aria-label and title attribute to the icon explaining that the icon is indicating an external link. But they'll also need to be translated, so you'll need to use the Eui18n component similar to this usage:

eui/src/components/breadcrumbs/breadcrumbs.tsx

Lines 96 to 111 in fab513d

const ellipsisButton = (

<EuiI18n

token="euiBreadcrumbs.collapsedBadge.ariaLabel"

default="Show all breadcrumbs">

{(ariaLabel: string) => (

<EuiBadge

aria-label={ariaLabel}

onClick={() => setIsPopoverOpen(!isPopoverOpen)}

onClickAriaLabel={ariaLabel}

title="View hidden breadcrumbs"

className="euiBreadcrumb euiBreadcrumb__collapsedBadge">

…

</EuiBadge>

)}

</EuiI18n>

);

Also, what do you think about making this size="s" so that it becomes more subtle?

I think the small size works a lot better

You should probably pass an aria-label and title attribute to the icon explaining that the icon is indicating an external link. But they'll also need to be translated, so you'll need to use the Eui18n component similar to this usage:

Added the aria-label, I tried adding a title but was getting this error:

cchaos

Ahh yeah, I forgot that SVG doesn't accept title attributes. We can just leave it as the aria-label for now.

One other thing is you should create another test case for this new property in the test file and update the snapshots. You should be able to just copy and paste and follow the pattern of the other props. I added that line item back to the checklist in the summary. Let me know if you need any help doing so.

cchaos · 2019-10-17T13:41:41Z

src/components/link/link.tsx

@@ -57,13 +61,30 @@ export type EuiLinkProps = ExclusiveUnion<
  EuiLinkAnchorProps
 >;

-const EuiLink = forwardRef<HTMLAnchorElement | HTMLButtonElement, EuiLinkProps>(
+const externalLinkIcon = (
+  <EuiI18n token="euiLink.external.ariaLabel" default="External Link">


Remember to use sentence case

Suggested change

<EuiI18n token="euiLink.external.ariaLabel" default="External Link">

<EuiI18n token="euiLink.external.ariaLabel" default="External link">

cchaos · 2019-10-17T13:43:07Z

src/components/link/link.tsx

@@ -111,6 +132,7 @@ const EuiLink = forwardRef<HTMLAnchorElement | HTMLButtonElement, EuiLinkProps>(
        ref={ref as React.Ref<HTMLAnchorElement>}
        {...anchorProps as EuiLinkAnchorProps}>
        {children}
+        {external ? externalLinkIcon : undefined}


Optional cleanup idea

Since the display of the external icon is tied to a property, you can also save some JS computing by setting the value of externalLinkIcon depending on the external property like so:

const externalLinkIcon = external ? ( <EuiI18n token="euiLink.external.ariaLabel" default="External link"> {...} </EuiI18n> ) : undefined;

and so then this line is simplified to be just

Suggested change

{external ? externalLinkIcon : undefined}

{externalLinkIcon}

@cchaos I did the optional cleanup you suggested. I was getting this error 64:26 error Unexpected use of 'external' no-restricted-globals so I did // eslint-disable-line no-restricted-globals Let me know if that's the way to go.

I added a comment below, but essentially, this code needs to be within the EuiLink component, not outside of it.

cchaos · 2019-10-17T16:49:38Z

CHANGELOG.md

@@ -16,6 +16,7 @@
 - Added `keyboardShorcut` glyph to 'EuiIcon ([#2413](https://github.com/elastic/eui/pull/2413))
 - Improved a11y in `EuiNavDrawer` ([#2417](https://github.com/elastic/eui/pull/2417))
 - Improved a11y in `EuiSuperDatePicker` ([#2426](https://github.com/elastic/eui/pull/2426))
+- Added `external` prop to `EuiLink` ([#2442](https://github.com/elastic/eui/pull/2410))


This CL line now needs to be moved up below master

cchaos · 2019-10-17T16:51:34Z

src/components/link/link.tsx

@@ -57,13 +61,32 @@ export type EuiLinkProps = ExclusiveUnion<
  EuiLinkAnchorProps
 >;

-const EuiLink = forwardRef<HTMLAnchorElement | HTMLButtonElement, EuiLinkProps>(
+const externalLinkIcon = external ? ( // eslint-disable-line no-restricted-globals


Sorry I didn't catch this before, but the reason you were getting the eslint error is because external only exists within EuiLink. You'll need to move this to within the component code before the return().

cchaos

LGTM, but there's a small error in the CL. Once that's fixed, it's good to merge!

cchaos · 2019-10-17T18:04:37Z

CHANGELOG.md

@@ -1,6 +1,7 @@
 ## [`master`](https://github.com/elastic/eui/tree/master)

 - Added ability for `EuiColorStops` to accept user-defined range bounds ([#2396](https://github.com/elastic/eui/pull/2396))
+- Added `external` prop to `EuiLink` ([#2442](https://github.com/elastic/eui/pull/2410))


Link is pointing to the wrong PR

andreadelrio and others added 6 commits October 15, 2019 22:23

adding external prop to euiLink

8b796f6

interface and props table fix

f65423a

Merge pull request #4 from thompsongl/external-link

7b0e385

[external-link] interface and props table fix

updated prop desc

2ee198e

external only for links with hrefs defined

22c6e34

cl

d8899fb

cchaos suggested changes Oct 16, 2019

View reviewed changes

added aria-label

8a36059

andreadelrio requested a review from cchaos October 16, 2019 20:20

cchaos suggested changes Oct 17, 2019

View reviewed changes

added test

5d67abe

andreadelrio requested a review from cchaos October 17, 2019 16:45

cchaos suggested changes Oct 17, 2019

View reviewed changes

andreadelrio added 2 commits October 17, 2019 12:07

fix cl and moved externalLinkIcon declaration

bf198d0

fix cl conflict

a7bec65

cchaos approved these changes Oct 17, 2019

View reviewed changes

fixed link in cl

b73f8eb

andreadelrio merged commit 5437331 into elastic:master Oct 17, 2019

thompsongl mentioned this pull request Oct 23, 2019

Upgrade EUI to 14.7.0 elastic/kibana#49110

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added external prop to EuiLink #2442

Added external prop to EuiLink #2442

andreadelrio commented Oct 16, 2019 •

edited by cchaos

Loading

cchaos left a comment

cchaos Oct 16, 2019

cchaos Oct 16, 2019

andreadelrio Oct 16, 2019

andreadelrio Oct 16, 2019 •

edited

Loading

cchaos left a comment

cchaos Oct 17, 2019

cchaos Oct 17, 2019

andreadelrio Oct 17, 2019

cchaos Oct 17, 2019

cchaos Oct 17, 2019

cchaos Oct 17, 2019

cchaos left a comment

cchaos Oct 17, 2019

	const ellipsisButton = (
	<EuiI18n
	token="euiBreadcrumbs.collapsedBadge.ariaLabel"
	default="Show all breadcrumbs">
	{(ariaLabel: string) => (
	<EuiBadge
	aria-label={ariaLabel}
	onClick={() => setIsPopoverOpen(!isPopoverOpen)}
	onClickAriaLabel={ariaLabel}
	title="View hidden breadcrumbs"
	className="euiBreadcrumb euiBreadcrumb__collapsedBadge">
	…
	</EuiBadge>
	)}
	</EuiI18n>
	);

	<EuiI18n token="euiLink.external.ariaLabel" default="External Link">
	<EuiI18n token="euiLink.external.ariaLabel" default="External link">

Added external prop to EuiLink #2442

Added external prop to EuiLink #2442

Conversation

andreadelrio commented Oct 16, 2019 • edited by cchaos Loading

Summary

Checklist

cchaos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andreadelrio Oct 16, 2019 • edited Loading

Choose a reason for hiding this comment

cchaos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cchaos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andreadelrio commented Oct 16, 2019 •

edited by cchaos

Loading

andreadelrio Oct 16, 2019 •

edited

Loading