Skip to content
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.

Sign up for GitHub

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

Jump to bottom

ExternalLink: Replace icon with unicode arrow #60255

Merged
merged 12 commits into from
Apr 3, 2024
Merged

ExternalLink: Replace icon with unicode arrow #60255

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
afc687a
Externalink: Replace icon with unicode arrow
jameskoster Mar 27, 2024
aa8424d
Changelog
jameskoster Mar 27, 2024
a236711
Apply underline only to text
jameskoster Mar 28, 2024
e395c81
Use scss and update spacing
jameskoster Mar 28, 2024
d586f79
Update packages/components/CHANGELOG.md
jameskoster Apr 2, 2024
60cf335
Add aria-label to arrow container
jameskoster Apr 2, 2024
f56bd17
Fix the arrow font weight to 400
jameskoster Apr 2, 2024
8b45f74
Merge branch 'trunk' into update/externalink
jameskoster Apr 2, 2024
bb308bc
Rebase
jameskoster Apr 3, 2024
7dfb1da
Update LinkControl test to account for ExternalLink markup changes
tyxla Apr 3, 2024
10ae57f
Fix the pattern overrides e2e test
tyxla Apr 3, 2024
b080dab
Merge branch 'trunk' into update/externalink
jameskoster Apr 3, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions packages/components/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,10 @@

## Unreleased

### Enhancements

- `Externalink`: Use unicode arrow instead of svg icon ([#60255](https://github.com/WordPress/gutenberg/pull/60255)).
jameskoster marked this conversation as resolved.
Show resolved Hide resolved

### Bug Fix

- `InputControl`: Ignore IME events when `isPressEnterToChange` is enabled ([#60090](https://github.com/WordPress/gutenberg/pull/60090)).
Expand Down
11 changes: 4 additions & 7 deletions packages/components/src/external-link/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,13 +9,11 @@ import type { ForwardedRef } from 'react';
*/
import { __ } from '@wordpress/i18n';
import { forwardRef } from '@wordpress/element';
import { external } from '@wordpress/icons';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do favor the idea of not relying on icons when we don't need to. It sure helps keep things lighter.


/**
* Internal dependencies
*/
import { VisuallyHidden } from '../visually-hidden';
import { StyledIcon } from './styles/external-link-styles';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I favor removal of Emotion when we can. 👍

We already had classnames used here, so perhaps we should have used SCSS anyway.

import type { ExternalLinkProps } from './types';
import type { WordPressComponentProps } from '../context';

Expand Down Expand Up @@ -66,17 +64,16 @@ function UnforwardedExternalLink(
rel={ optimizedRel }
ref={ ref }
>
{ children }
<span className="components-external-link__contents">
{ children }
</span>
<VisuallyHidden as="span">
{
/* translators: accessibility text */
__( '(opens in a new tab)' )
}
</VisuallyHidden>
<StyledIcon
icon={ external }
className="components-external-link__icon"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens with plugins that were relying on the components-external-link__icon classname to add styles to the icon?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wrapped the symbol in a span with this class, so if any plugins are targeting it their styles will be applied.

/>
<span className="components-external-link__icon">&#8599;</span>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't want this arrow character to be read out literally in screen readers. How about we move the accessibility text to the aria-label?

diff --git a/packages/components/src/external-link/index.tsx b/packages/components/src/external-link/index.tsx
index 7974560d78..3f09cdd92b 100644
--- a/packages/components/src/external-link/index.tsx
+++ b/packages/components/src/external-link/index.tsx
@@ -13,7 +13,6 @@ import { forwardRef } from '@wordpress/element';
 /**
  * Internal dependencies
  */
-import { VisuallyHidden } from '../visually-hidden';
 import type { ExternalLinkProps } from './types';
 import type { WordPressComponentProps } from '../context';
 
@@ -67,13 +66,15 @@ function UnforwardedExternalLink(
 			<span className="components-external-link__contents">
 				{ children }
 			</span>
-			<VisuallyHidden as="span">
-				{
+			<span
+				className="components-external-link__icon"
+				aria-label={
 					/* translators: accessibility text */
 					__( '(opens in a new tab)' )
 				}
-			</VisuallyHidden>
-			<span className="components-external-link__icon">&#8599;</span>
+			>
+				&#8599;
+			</span>
 		</a>
 		/* eslint-enable react/jsx-no-target-blank */
 	);

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a great idea!

</a>
/* eslint-enable react/jsx-no-target-blank */
);
Expand Down
11 changes: 11 additions & 0 deletions packages/components/src/external-link/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
.components-external-link {
text-decoration: none;
}

.components-external-link__contents {
text-decoration: underline;
}

.components-external-link__icon {
margin-left: 0.5ch;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't that still unpredictable and varying? I wonder if we'd instead prefer a more fixed value that doesn't change based on the font.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a strong feeling, but I like that using ch means the spacing is relative to the font.

If we used a fixed space then the gap could end up feeling too large for condensed font variants, or too small for extended ones.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good, as long as it's intentional 👍

}
17 changes: 0 additions & 17 deletions packages/components/src/external-link/styles/external-link-styles.ts
View file
Open in desktop

This file was deleted.

1 change: 1 addition & 0 deletions packages/components/src/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
@import "./dropdown-menu/style.scss";
@import "./duotone-picker/style.scss";
@import "./duotone-picker/color-list-picker/style.scss";
@import "./external-link/style.scss";
@import "./form-toggle/style.scss";
@import "./form-token-field/style.scss";
@import "./guide/style.scss";
Expand Down
Loading