-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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.
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
ExternalLink: Replace icon with unicode arrow #60255
Conversation
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.
To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
Agreed. |
Unsure if it's the best way to do it, but I pushed a commit which applies the underline only to text inside |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally, I like the idea of using lighter and built-in alternatives for whatever we can. 👍
I do have some concerns though - left some comments, let me know what you think.
icon={ external } | ||
className="components-external-link__icon" | ||
/> | ||
↗ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd advise against spacing with
because it makes it way more difficult to define equal spacing across all browsers and to precisely define the margin we want to have on the left side of the icon. In general, using content features for styling purposes is discouraged for the same reasons.
icon={ external } | ||
className="components-external-link__icon" | ||
/> | ||
↗ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any concerns with font browser support? Are we certain that the user won't use a font that doesn't support this unicode arrow icon?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good!
<VisuallyHidden as="span"> | ||
{ | ||
/* translators: accessibility text */ | ||
__( '(opens in a new tab)' ) | ||
} | ||
</VisuallyHidden> | ||
<StyledIcon | ||
icon={ external } | ||
className="components-external-link__icon" |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
> | ||
{ children } | ||
<span style={ { textDecoration: 'underline' } }>{ children }</span> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer defining those styles with SCSS or Emotion rather than with CSS. Inlining all those styles could get messy.
@@ -9,13 +9,11 @@ import type { ForwardedRef } from 'react'; | |||
*/ | |||
import { __ } from '@wordpress/i18n'; | |||
import { forwardRef } from '@wordpress/element'; | |||
import { external } from '@wordpress/icons'; |
There was a problem hiding this comment.
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'; |
There was a problem hiding this comment.
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.
> | ||
{ children } | ||
<span style={ { textDecoration: 'underline' } }>{ children }</span> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, this will break a bunch of tests that expect the link text to be a direct child of the link.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
span
s inside a
is valid html. So this would just require the tests to be updated, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tests are failing due to a string mismatch at the accessibility tree level, so it should be fixed by #60255 (comment), I think. (It's good that they failed and flagged the issue 🙂)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, after addressing Lena's abovementioned comment, things should be good.
Size Change: -103 B (0%) Total Size: 1.73 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice enhancement! Looks good to me once the screen reader issue is resolved.
<VisuallyHidden as="span"> | ||
{ | ||
/* translators: accessibility text */ | ||
__( '(opens in a new tab)' ) | ||
} | ||
</VisuallyHidden> | ||
<StyledIcon | ||
icon={ external } | ||
className="components-external-link__icon" | ||
/> | ||
<span className="components-external-link__icon">↗</span> |
There was a problem hiding this comment.
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">↗</span>
+ >
+ ↗
+ </span>
</a>
/* eslint-enable react/jsx-no-target-blank */
);
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good 👍
Feel free to ship once the a11y suggestion by Lena is addressed and the tests are green again 🚀
<VisuallyHidden as="span"> | ||
{ | ||
/* translators: accessibility text */ | ||
__( '(opens in a new tab)' ) | ||
} | ||
</VisuallyHidden> | ||
<StyledIcon | ||
icon={ external } | ||
className="components-external-link__icon" | ||
/> | ||
<span className="components-external-link__icon">↗</span> |
There was a problem hiding this comment.
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!
} | ||
|
||
.components-external-link__icon { | ||
margin-left: 0.5ch; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 👍
icon={ external } | ||
className="components-external-link__icon" | ||
/> | ||
↗ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good!
> | ||
{ children } | ||
<span style={ { textDecoration: 'underline' } }>{ children }</span> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, after addressing Lena's abovementioned comment, things should be good.
Nice. If we do this the icon should always have the same font weight, though. I.e. never be bold, always normal. |
Co-authored-by: Lena Morita <lena@jaguchi.com>
Looks like the tests will need updating here, if someone could assist with that I'd appreciate it 🤞 |
7dfb1da should do it. It's a basic change that reflects the fact that we now have an extra wrapper element there. |
There was also an e2e failure, which 10ae57f should resolve. |
Thank you :) |
Co-authored-by: jameskoster <jameskoster@git.wordpress.org> Co-authored-by: tyxla <tyxla@git.wordpress.org> Co-authored-by: mirka <0mirka00@git.wordpress.org> Co-authored-by: richtabor <richtabor@git.wordpress.org> Co-authored-by: jasmussen <joen@git.wordpress.org>
We've run into an issue on Link Control, where the arrow is not visible on an external link. It's kind of an odd case, but previously the SVG was hidden manually via CSS. But with this iteration, that's not possible, as the arrow also announces the link opening in a new tab. Thoughts on what we should do to resolve this? #60439 (review) |
Sorry @richtabor I missed that comment! So the arrow needs to be visually hidden, but still announced by screen readers? Could we use For something neater would it make sense to add a prop to |
I think the main point of If you're sure about the accessibility implications of not showing an arrow in a specific use case context (like if you're sure there are enough context clues that it would open in a new tab), it might be better to just roll your own |
Yea, I don't think a specific prop for this is necessary. I'll probably use clip to hide it—it's probably fine. Thanks for hashing this with me team. |
What?
Use a unicode arrow symbol rather than the
external
icon inExternalink
.Why?
Before
After
If we agree this is a good idea there could be some further refinements to try. For instance the arrow might work better without the underline: