Skip to content

Conversation

@up1512001
Copy link
Member

@up1512001 up1512001 commented Jun 18, 2024

What?

  • updated aria label text as site text

Why?

Fixes #62644

How?

  • using site title for aria label label={ decodeEntities( siteTitle ) }

Screenshots or screencast

Screenshot 2024-06-18 at 15 42 47
Screen.Recording.2024-06-18.at.23.00.18.mov

@github-actions
Copy link

github-actions bot commented Jun 18, 2024

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 props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: up1512001 <up1512001@git.wordpress.org>
Co-authored-by: afercia <afercia@git.wordpress.org>
Co-authored-by: richtabor <richtabor@git.wordpress.org>
Co-authored-by: Mamaduka <mamaduka@git.wordpress.org>
Co-authored-by: youknowriad <youknowriad@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

@skorasaurus skorasaurus added the [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). label Jun 18, 2024
@youknowriad youknowriad added the Needs Accessibility Feedback Need input from accessibility label Jun 18, 2024
@youknowriad youknowriad requested review from afercia and joedolson June 18, 2024 19:29
@youknowriad youknowriad added the Needs Design Feedback Needs general design feedback. label Jun 18, 2024
@youknowriad youknowriad requested a review from richtabor June 18, 2024 19:31
@youknowriad
Copy link
Contributor

I think @richtabor worked on this recently, so this might be a design regression a bit.

@afercia afercia added the [Type] Bug An existing feature does not function as intended label Jun 19, 2024
Copy link
Contributor

@afercia afercia left a 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.

@up1512001
Copy link
Member Author

@afercia addressed your feedback please review the PR.

@up1512001
Copy link
Member Author

@afercia updated as per your feedback. please check.

Copy link
Member

@richtabor richtabor left a 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.

@up1512001
Copy link
Member Author

@richtabor @afercia I have reverted style-related changes and only kept accessibility related changes.

@afercia
Copy link
Contributor

afercia commented Jul 5, 2024

@up1512001 thank you. Looks good to me 👍🏽
I will creat a follow-up issue for the CSS pseudo element.

Copy link
Contributor

@afercia afercia left a comment

Choose a reason for hiding this comment

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

LGTM

@up1512001
Copy link
Member Author

@afercia I thought we should use the wordpress icon for consistency instead of the after pseudo element so whenever the icon is updated it will reflect to everywhere.
Let me know your thoughts.

@afercia
Copy link
Contributor

afercia commented Jul 5, 2024

+1 when @richtabor approves.

@afercia afercia requested a review from richtabor July 5, 2024 09:06
@afercia
Copy link
Contributor

afercia commented Jul 5, 2024

@afercia I thought we should use the wordpress icon for consistency instead of the after pseudo element so whenever the icon is updated it will reflect to everywhere.

@up1512001 personally I don't like the design of this component and I don't understand the need to replace the external icon with the North East Arrow unicode character. If it was up to me, I would change it but hte prevalent opinion from design is to keep the arrow so yes, fair enough 🙂

@richtabor
Copy link
Member

richtabor commented Jul 7, 2024

I don't understand the need to replace the external icon with the North East Arrow unicode character.

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.

@afercia
Copy link
Contributor

afercia commented Jul 10, 2024

Making it smaller made it less communicative/complex.

Making it smaller made it also less readable. See #62832 for a more in depth analysis.

@up1512001
Copy link
Member Author

Hi @richtabor any feedback for this PR?

@Mamaduka Mamaduka added the props-bot Manually triggers Props Bot to ensure the list of props is up to date. label Aug 3, 2024
@github-actions github-actions bot removed the props-bot Manually triggers Props Bot to ensure the list of props is up to date. label Aug 3, 2024
@Mamaduka Mamaduka merged commit 5cb1e2e into WordPress:trunk Aug 3, 2024
@github-actions github-actions bot added this to the Gutenberg 19.0 milestone Aug 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

[Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). Needs Accessibility Feedback Need input from accessibility Needs Design Feedback Needs general design feedback. [Type] Bug An existing feature does not function as intended

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Site title: link text and accessible name mismatch

6 participants