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

Site Editor Navigation panel: Link titles are misleading #48885

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

Conversation

shreyasikhar
Copy link
Contributor

What?

Previously, the button titles were showing unrelated titles on the page links under navigation. Now with this PR, it adds more specific titles to the pages under navigation without disturbing the block titles for block links.

Why?

Issue - #48799

How?

Added some extra checks for links under navigation.
If the link is from the WordPress page, then it will show the title as View {pageName}, else it will show Edit {blockName} block as the title.

Testing Instructions

  1. Login to WordPress dashboard
  2. Go to Appearance -> Editor -> Navigation
  3. Toggle the Page List by clicking the caret icon.
  4. Hover on any of the list item under Page List to check the title.

Testing Instructions for Keyboard

Screenshots or screencast

Screen.Recording.2023-03-07.at.6.00.25.PM.mov

Comment on lines 73 to 81
editAriaLabel =
block.attributes?.id && block.attributes?.label
? sprintf(
// translators: %s: The title of the page.
__( 'View %s' ),
block.attributes?.label
)
: editAriaLabel;

Copy link
Contributor

Choose a reason for hiding this comment

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

Can I check this has been tested in the Nav block list view as well that appears in the Inspector controls when selecting the Navigation block?

This exhibits different behaviour - namely when you click an item it opens the block settings for that block. Therefore does "View" make sense in that context?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can I check this has been tested in the Nav block list view as well that appears in the Inspector controls when selecting the Navigation block?

I am not sure if I understood it correctly, but @getdave can you please check below screenshot. Is that what you wanted to check? Apologies if I misunderstood it.
Screenshot 2023-03-07 at 7 38 50 PM

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah no I mean this thing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @getdave
In the list view as well, it is showing the same updates in the title.

@getdave
Copy link
Contributor

getdave commented Mar 7, 2023

Thank you for the PR. I'm tagging in a few folks who work regularly on the Nav block to review.

@getdave getdave added [Block] Navigation Affects the Navigation Block [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [a11y] Labelling labels Mar 7, 2023
@jameskoster
Copy link
Contributor

jameskoster commented Mar 7, 2023

Thanks for working on this. It definitely feels like an improvement in the Navigation panel:

Before

before.mp4

After

after.mp4

I appreciate it's tricky since it's the same underlying tech, but ideally the title should remain the same in the Navigation Inspector:

Screenshot 2023-03-07 at 14 16 17

Side note: I suspect it will fall into #48675, but it would be nice if we could do something with blocks like Site Logo, Page List as well. Those titles are still misleading.

@carolinan
Copy link
Contributor

Do we not need to pass a prop with a context (the context being sidebar navigation), so that we can determine where the OffCanvasEditor is used, and then use that to display different labels?

@scruffian
Copy link
Contributor

I think we can use the onSelect prop which is already passed. If the block passes an onSelect prop then we know that the parent component is handling the selection, so we can infer the label from that. I'm happy to spin up a PR if you like?

@shreyasikhar
Copy link
Contributor Author

I think we can use the onSelect prop which is already passed. If the block passes an onSelect prop then we know that the parent component is handling the selection, so we can infer the label from that. I'm happy to spin up a PR if you like?

@scruffian I have used onSelect to keep link titles different in sidebar-navigation and in Navigation block list view. Please let me know if this is what you are expecting and sorry for the delayed response.

I agree with @jameskoster on handling blocks like Site Logo, Page List etc. separately. I would be happy if I get some suggestions on that from you all.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Navigation Affects the Navigation Block [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes).
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants