-
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
Fix: a11y text for site editor #62648
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. |
I think @richtabor worked on this recently, so this might be a design regression a bit. |
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.
Please see inline comments.
@afercia addressed your feedback please review the PR. |
@afercia updated as per your feedback. please check. |
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 think @richtabor worked on this recently, so this might be a design regression a bit.
Yes, let's resolve the a11y feedback apart from icon changes.
The ExternalLink component has been updated to leverage the ↗ unicode character. This is intended to share the same DNA as that component, as it is also a text only external link.
@richtabor @afercia I have reverted style-related changes and only kept accessibility related changes. |
@up1512001 thank you. Looks good to me 👍🏽 |
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.
LGTM
@afercia I thought we should use the wordpress icon for consistency instead of the |
+1 when @richtabor approves. |
@up1512001 personally I don't like the design of this component and I don't understand the need to replace the |
It's much more palatable when placed directly alongside text. The external icon was too big and disorienting when in those placements. Making it smaller made it less communicative/complex. I'm fine if we do the unicode symbol as an icon, resulting in no change visually. |
Making it smaller made it also less readable. See #62832 for a more in depth analysis. |
Hi @richtabor any feedback for this PR? |
What?
Why?
Fixes #62644
How?
label={ decodeEntities( siteTitle ) }
Screenshots or screencast
Screen.Recording.2024-06-18.at.23.00.18.mov