Links that open in a new tab/window need to inform users a new tab/window will open #1577

mehigh · 2017-06-29T08:00:25Z

Description

Fixes #1105 by adding the external dashicon and the standard screen reader text too.

How Has This Been Tested?

I checked that the Excerpt external link which makes use of the new ExternalLink component contains the new icon and displays it correctly in major OSX browsers (Safari, Chrome, Firefox).
I used the browser's inspector to confirm the screen-reader-text element gets properly added as well.

Screenshots:

Types of changes

New feature (non-breaking change which adds functionality)

Checklist:

My code is tested.
My code follows the WordPress code style.
My code follows has proper inline documentation. (Other as basic components don't have inline documentation, just like this one)

ellatrix · 2017-06-29T11:37:53Z

Is this the correct way to do this? I wish we could use CSS and use target="_blank", otherwise, could we make a shared component for this?

afercia · 2017-06-29T13:04:04Z

could we make a shared component for this?

In another project at Yoast we've actually built a simple <NewTabMessage> component. It's a simple span with a translatable text and visually hidden by default. No icon in our case. Here, the icon could be passed as a prop, maybe.

mehigh · 2017-06-29T13:07:28Z

@iseulde
The icon can come through via CSS, but the added mark-up (the screen reader extra text) is required either way. And since this is the only occurrence in the Guttenberg interface, I think it can live fine by itself as it is until there's a need for a second usage.

afercia · 2017-06-29T13:50:34Z

I think PostPermalink uses a second target="_blank".

mehigh · 2017-06-30T15:10:27Z

Thank you, afercia. I'll see whether adding the external icon looks OK in the PostPermalink area, and if it does I'll add it in and also create a component for them.

mehigh · 2017-07-01T12:54:10Z

I think PostPermalink uses a second target="_blank".

I looked all over the interface of Guttenberg, but I failed to find the PostPermalink area.

I checked how it's done in the regular posts editing screen:

And the page loads in the same tab, hence given the standard functionality an external icon is not required on that element.

mehigh · 2017-07-01T13:13:08Z

I've exported the newWindowIcon into its own component, but I didn't find a way to use it the 2nd time yet.

ellatrix · 2017-07-01T13:16:13Z

I'm sure there will be. What I actually meant is something like <ExternalLink href="..." />.

mehigh · 2017-07-01T13:22:10Z

I've stopped using the inline SVG and instead used CSS to add the icon and it offers an improved UX because now the icon too changes color on hover to match the color of the link text.

Icon matches link text color:

ellatrix · 2017-07-01T13:22:40Z

I think we do want to keep the SVG instead of using the icon font? Cc @jasmussen

mehigh · 2017-07-01T13:26:18Z

Change of heart @iseulde ? (you previously mentioned "I wish we could use CSS").
With an SVG file inlined, there's no reason to use any CSS whatsoever then for the icon.

I saw the inlined SVG icons are used almost everywhere, but also saw that the dashicons font is enqueued in the guttenberg page, so... let me know and I'll add the SVG back in or not when I go about creating the ExternalLink component to replace the externalIcon I just created.

ellatrix · 2017-07-01T13:27:44Z

Yeah, I wish we could use CSS. 🙂

jasmussen · 2017-07-01T13:49:18Z

Let's avoid the icon font if we can. Pretty sure this ticket is a duplicate, and Andrew Duthie, had an idea for how to add "external" as a property that would simply output the icon after the link. It sounded pretty good to me, and so long as the js code is simple, it's cool.

jasmussen · 2017-07-01T13:51:25Z

Doh, sorry, didn't realize this was a PR referencing the very issue I was talking about. My apologies, I blame being mobile.

mehigh · 2017-07-01T15:00:46Z

Understood, jasmussen. Yes, the iconfont can be avoided. I'll update the PR accordingly.

mehigh · 2017-07-01T19:39:03Z

I've resurrected the usage of the SVG and dropped the iconfont according to @jasmussen 's input.
I also created the ExternalLink component according to @iseulde 's input. This covers the addition of the icon and the accessibility text.

Thank you for all your recommendations.

ellatrix · 2017-07-01T21:09:13Z

components/external-link/index.js

+	return (
+		<a { ...additionalProps } href={ href } target="_blank">
+			{ children }
+			<span className="new-window-icon">


Is there a need for this span? Can the class not be put on the parent?

@iseulde
This isn't needed, I'll update the PR to include it on the anchor tag instead and drop the span.

ellatrix · 2017-07-01T21:13:20Z

components/external-link/style.scss

+		height: 1.4em;
+		vertical-align: top;
+	}
+}


We don't have checks for it, but: https://github.com/WordPress/gutenberg/blob/v0.3.0/.editorconfig#L9. Is there any way to add checks @nylen or @ntwb?

Yes, it's checked as part of the default stylelint config:

https://github.com/WordPress-Coding-Standards/stylelint-config-wordpress/blob/master/index.js#L71

mehigh · 2017-07-03T05:13:17Z

I've handled the latest update and also updated the PR description to match the PULL_REQUEST_TEMPLATE.md I've seen, such that it's easier to get all of this in.

youknowriad · 2017-07-03T09:04:52Z

components/external-link/index.js

+import Dashicon from '../dashicon';
+import './style.scss';
+
+function ExternalLink( { href, children, className, ...additionalProps } ) {


Should we drop href (it's not used here, and could be included by ...additionalProps

I intentionally passed that through the href={ href } because without it the ExternalLink becomes useless (opens up a new empty tab). I know it's not a real required parameter, but it felt better this way.

nylen · 2017-07-03T09:54:24Z

I'd say leave `href`, it's such a fundamental property of a link that you want to see it in the code.

…

On Jul 3, 2017 11:05 AM, "Riad Benguella" ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In components/external-link/index.js <#1577 (comment)>: > + * External dependencies + */ +import classnames from 'classnames'; + +/** + * WordPress dependencies + */ +import { __ } from 'i18n'; + +/** + * Internal dependencies + */ +import Dashicon from '../dashicon'; +import './style.scss'; + +function ExternalLink( { href, children, className, ...additionalProps } ) { Should we drop href (it's not used here, and could be included by ...additionalProps — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1577 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAN2zsJsUEu54Jh0SNyuwZsj6dhwD8OVks5sKK7MgaJpZM4OI85u> .

mehigh · 2017-07-12T17:25:39Z

@iseulde any steps I can take to allow this development to more forward and not get stale?

mehigh · 2017-07-17T07:05:34Z

I've refreshed the PR with the latest in master (a small conflict was popping up).

aduth

This is looking quite good, but I'd suggest the addition of a rel attribute.

First for the external link type, see also:

https://developer.mozilla.org/en-US/docs/Web/HTML/Link_types

Second for noopener and noreferrer, see also:

We can can join with incoming prop value:

function ExternalLink( { href, children, className, rel = '', ...additionalProps } ) {
	rel = uniq( compact( [
		...rel.split( ' ' ),
		'external',
		'noreferrer',
		'noopener'
	] ) ).join( ' ' );

https://lodash.com/docs/4.17.4#uniq
https://lodash.com/docs/4.17.4#compact

aduth · 2017-07-17T12:23:54Z

components/external-link/style.scss

@@ -0,0 +1,7 @@
+.new-window-icon {
+	.dashicon {


A bit of horizontal margin might be nice, but is minor and can be addressed in a styling pass if necessary.

Thanks. I'll make that happen. Good point with the noopener/etc. rels.

…ndow will open ## Description Fixes WordPress#1105 by adding the external dashicon and the standard screen reader text too. ## How Has This Been Tested? I checked that the Excerpt external link which makes use of the new ExternalLink component contains the new icon and displays it correctly in major OSX browsers (Safari, Chrome, Firefox). I used the browser's inspector to confirm the screen-reader-text element gets properly added as well. ## Screenshots: ![Updated panel](http://files.urldocs.com/share/excerpt-block-updated/excerpt-bloc k-updated.png) ## Types of changes New feature (non-breaking change which adds functionality) ## Checklist: - [x] My code is tested. - [x] My code follows the WordPress code style. - [x] My code follows has proper inline documentation. (Other as basic components don't have inline documentation, just like this one)

mehigh · 2017-07-18T09:01:41Z

@aduth

Your input has been handled. I've also squashed my changes in a single commit and updated this to the latest in master.
I've added the rels and a small margin on the sides of the icon - looks better this way.

Thanks for the resource links, they've been really handy.

aduth

This is looking good, and I think could be merged after the class name change.

aduth · 2017-07-18T12:40:25Z

components/external-link/index.js

+		'noreferrer',
+		'noopener',
+	] ) ).join( ' ' );
+	const classes = classnames( 'new-window-icon', className );


Per recommended CSS Naming guidelines, the default class name should be components-external-link

https://github.com/WordPress/gutenberg/blob/master/docs/coding-guidelines.md#naming

aduth · 2017-07-18T12:42:57Z

components/external-link/index.js

+	return (
+		<a { ...additionalProps } className={ classes } href={ href } target="_blank" rel={ rel }>
+			{ children }
+			<span className="screen-reader-text">{ __( '(opens in a new window)' ) }</span>


Minor: Elsewhere in the WordPress codebase, this string is preceded with a translator comment translators: accessibility text. My main concern would be if omitting the translator comment would create a separate string entry in GlotPress (I'm not sure)

https://translate.wordpress.org/projects/wp/dev/fr/default?filters%5Bterm%5D=opens+in+a+new+window&filters%5Buser_login%5D=&filters%5Bstatus%5D=current_or_waiting_or_fuzzy_or_untranslated&filter=Filter&sort%5Bby%5D=priority&sort%5Bhow%5D=desc

Otherwise this could be:

<span className="screen-reader-text"> { /* translators: accessibility text */ __( '(opens in a new window)' ) } </span>

…comment

mehigh · 2017-07-18T12:52:13Z

@aduth
Feedback handled.

aduth

Looks great, thanks!

ellatrix requested a review from afercia June 29, 2017 11:36

ellatrix added the [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). label Jun 29, 2017

ellatrix reviewed Jul 1, 2017

View reviewed changes

youknowriad reviewed Jul 3, 2017

View reviewed changes

aduth requested changes Jul 17, 2017

View reviewed changes

mehigh force-pushed the feature/1105 branch from aa60c02 to 0a39d47 Compare July 18, 2017 08:58

1105 - Fix formatting

e10850f

aduth reviewed Jul 18, 2017

View reviewed changes

1105 - Changed the class name. Added translators: accessibility text …

fa21a0f

…comment

aduth approved these changes Jul 18, 2017

View reviewed changes

aduth merged commit 710fb34 into WordPress:master Jul 18, 2017

delawski mentioned this pull request Dec 3, 2021

Do async site scan on plugin activation instead of synchronous validation ampproject/amp-wp#6685

Merged

2 tasks

Links that open in a new tab/window need to inform users a new tab/window will open #1577

Links that open in a new tab/window need to inform users a new tab/window will open #1577

Conversation

mehigh commented Jun 29, 2017 • edited Loading

Description

How Has This Been Tested?

Screenshots:

Types of changes

Checklist:

ellatrix commented Jun 29, 2017

afercia commented Jun 29, 2017

mehigh commented Jun 29, 2017

afercia commented Jun 29, 2017

mehigh commented Jun 30, 2017

mehigh commented Jul 1, 2017

mehigh commented Jul 1, 2017

ellatrix commented Jul 1, 2017

mehigh commented Jul 1, 2017

ellatrix commented Jul 1, 2017

mehigh commented Jul 1, 2017

ellatrix commented Jul 1, 2017

jasmussen commented Jul 1, 2017

jasmussen commented Jul 1, 2017

mehigh commented Jul 1, 2017

mehigh commented Jul 1, 2017 • edited Loading

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

mehigh commented Jul 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nylen commented Jul 3, 2017 via email

mehigh commented Jul 12, 2017

mehigh commented Jul 17, 2017

aduth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mehigh commented Jul 18, 2017

aduth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mehigh commented Jul 18, 2017

aduth left a comment

Choose a reason for hiding this comment

mehigh commented Jun 29, 2017 •

edited

Loading

mehigh commented Jul 1, 2017 •

edited

Loading