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.

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

Add link to options-general.php in the site title description #31122

Open
wants to merge 4 commits into
base: trunk
Choose a base branch
from

Conversation

vdwijngaert
Copy link
Member

This adds a link to the general settings page by making use of createInterpolateElement, as described in #30871.

@vdwijngaert vdwijngaert added [Block] Site Title Affects the Site Title Block [Type] Enhancement A suggestion for improvement. labels Apr 28, 2021
Copy link
Contributor

@priethor priethor left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks for the PR! 👍

Apr-28-2021.13-27-44.mp4

@vdwijngaert vdwijngaert self-assigned this Apr 28, 2021
Copy link
Contributor

@carolinan carolinan left a comment

Choose a reason for hiding this comment

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

Hi!
If this must be opened in a new tab, then there needs to be a notification for screen reader users that the link opens in a new tab. This can be visible (preferred) or
for example an aria-label or https://developer.wordpress.org/block-editor/reference-guides/components/visually-hidden/

The text used for the message is:

/* translators: accessibility text */
__( '(opens in a new tab)' )

(https://make.wordpress.org/accessibility/handbook/content/link-destinations/#the-link-opens-in-a-new-window-or-tab)

I also noticed that with NVDA screen reader, > was read out as "greater than". Is this way of describing the path used elsewhere?

@vdwijngaert
Copy link
Member Author

Hi!
If this must be opened in a new tab, then there needs to be a notification for screen reader users that the link opens in a new tab. This can be visible (preferred) or
for example an aria-label or https://developer.wordpress.org/block-editor/reference-guides/components/visually-hidden/

The text used for the message is:

/* translators: accessibility text */
__( '(opens in a new tab)' )

(https://make.wordpress.org/accessibility/handbook/content/link-destinations/#the-link-opens-in-a-new-window-or-tab)

I also noticed that with NVDA screen reader, > was read out as "greater than". Is this way of describing the path used elsewhere?

Valid points. I'll look into it!

@vdwijngaert
Copy link
Member Author

vdwijngaert commented Apr 30, 2021

Hey @carolinan, this is how Google suggests this should be handled in their developer documentation style guide:

Wrap the angle bracket with a span tag and add an aria-label attribute with and then text (<span aria-label="and then">></span>). Otherwise, some screen readers might read > as greater than.

I can do it like that, or should this be made into something more reusable and less confusing for developers and translators?

If this must be opened in a new tab

I'm not sure. Do you think it should be opened in a new tab?

@carolinan
Copy link
Contributor

carolinan commented May 3, 2021

I am trying to think of other places where links lead from the editor to the admin area.
The link "Manage reusable blocks" does not open in a new window.
So I don't think it should open in a new window.

@carolinan
Copy link
Contributor

Yes I think an aria-label would be good for the symbol. The documentation style guide also recommends making the text bold.
https://make.wordpress.org/docs/style-guide/formatting/procedures/#instructions-with-multiple-actions

@vdwijngaert vdwijngaert force-pushed the add/site-title-link-to-settings-page branch from 3abcae7 to a564edd Compare May 10, 2021 06:46
@vdwijngaert
Copy link
Member Author

Okay, so I gave a go at a component for describing accessible steps. Would love some feedback on the way this would now be handled. Could you take a look, @carolinan?

path: (
<AccessibleSteps
elements={ [ __( 'Settings' ), __( 'General' ) ] }
link="options-general.php"
Copy link
Contributor

Choose a reason for hiding this comment

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

We'll want to avoid a hardcoded link here since some clients need to be able to override this link. Other places have used a slot/fill pattern, but we may need a new generic filter for links added in block settings.

Added a screencap of feedback to save folks a click, from #30926

Screen Shot 2021-05-10 at 9 11 15 AM

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for sharing! So we need a way for clients to be able to override internal links. Maybe something for a [Type] Discussion issue?

How do you suggest we go from here?

Copy link
Contributor

@gwwar gwwar May 10, 2021

Choose a reason for hiding this comment

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

For next steps maybe something like:

  1. Audit to see if any other hardcoded links are currently present in block settings
  2. Open up an exploration PR(s) to explore the customization hook we're adding, with a few links to test it out (anything that the audit uncovered + site title + home link block).

If you're interested in the general problem, I think we can reuse this PR and update the summary + title. If not, I was intending to follow up on this, and I'd be happy to ping folks when a mechanism is available. Just let me know :D

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Site Title Affects the Site Title Block [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Site Title: Add a link to the block description instructions to go to the described location.
4 participants