-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
base: trunk
Are you sure you want to change the base?
Conversation
editAriaLabel = | ||
block.attributes?.id && block.attributes?.label | ||
? sprintf( | ||
// translators: %s: The title of the page. | ||
__( 'View %s' ), | ||
block.attributes?.label | ||
) | ||
: editAriaLabel; | ||
|
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.
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?
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.
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.
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.
Ah no I mean this thing
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.
Thanks @getdave
In the list view as well, it is showing the same updates in the title.
Thank you for the PR. I'm tagging in a few folks who work regularly on the Nav block to review. |
Thanks for working on this. It definitely feels like an improvement in the Navigation panel: Beforebefore.mp4Afterafter.mp4I appreciate it's tricky since it's the same underlying tech, but ideally the title should remain the same in the Navigation Inspector: 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. |
Do we not need to pass a prop with a context (the context being sidebar navigation), so that we can determine where the |
I think we can use the |
@scruffian I have used I agree with @jameskoster on handling blocks like |
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 showEdit {blockName} block
as the title.Testing Instructions
Testing Instructions for Keyboard
Screenshots or screencast
Screen.Recording.2023-03-07.at.6.00.25.PM.mov