[Navigation] Makes URL dynamic for pages and posts #21050
Conversation
draganescu
added
[Type] Enhancement
draganescu
requested review from
ajitbohra,
jorgefilipecosta,
Soean and
talldan
as code owners
|
Size Change: +458 B (0%) Total Size: 1.14 MB
ℹ️ View Unchanged
|
|
This makes the URL dynamic for pages and posts on rendering the block but does not handle the URL stored as an attribute for pages and posts. I wonder how this can be solved though?
Just to be clear, the problem is when we have a menu saved and settings change (say permalink structure) and someone edits a menu item, they should see the updated link. cc @mtias |
draganescu
force-pushed
the
add/dynamic-links-navigation
branch
from
970952f to
40d3dd2
Compare
draganescu
force-pushed
the
add/dynamic-links-navigation
branch
from
20558b6 to
9996817
Compare
|
This PR also adds to the Navigation block the small feat of rendering a |
packages/e2e-tests/specs/experiments/__snapshots__/navigation.test.js.snap
Outdated
Show resolved
Hide resolved
|
@talldan I added back the link to the navigation item. Once the snapshots work I would suggest we address this stale url stored as attribute in a separate PR. For now the I am afraid that updating LinkControl for this case would make things complex in this context. What do you think? |
draganescu
force-pushed
the
add/dynamic-links-navigation
branch
from
6338640 to
b4dd7ee
Compare
|
So just to be clear, as I don't think it's all that clear for those outside this conversation—the bug this introduces is as follows:
The current behavior in |
|
Looking at the As far as the
The problem here is that all the parent components implementing Even if Assuming we only update the navigation items to not store any URL since the front will potentially render another one, we run into displaying links with no link, or invalid anchors styled to look like links. Even if we solve it for one block, say we customize only the cc @mtias what do you think of this? |
|
So many of these types of issues are related to this insistence on storing nav menus as serialized block data. We have an existing menu data storage method in WordPress that already has problems like these solved. I still don't really understand the reasoning for abandoning it. |
draganescu
force-pushed
the
add/dynamic-links-navigation
branch
from
b4dd7ee to
1973247
Compare
I don't think We should only handle the Link block as used in Navigation for now. I'd imagine
@earnjam I think the issue is that the current setup addresses some issues but leaves quite a few others open. Once we start allowing more blocks to be inserted in menus — search, social icons, headings, images, text — the storage breaks down a bit as it needs to handle many other representations, some dynamic and some static. Depending on how the design evolves, we might need to have specialized blocks for any set of links (like sub-pages) so that they can be better intermixed with other blocks. In any case, the navigation block currently works with both data structures depending on where it's used (site editing or nav-menus.php screen) so we can see where the different issues arise. Also worth emphasizing for this particular thread that the Link block is not necessarily attached to navigation, it could become a standalone block for various other uses, so it still needs to model the attributes properly. |
draganescu
force-pushed
the
add/dynamic-links-navigation
branch
from
d9b4caf to
6207832
Compare
|
@talldan can you check this again, I added your suggestion to pass the id as part of value, good idea! Thank you! |
|
Here's what I see in the post editor after applying this PR:
Interestingly, the block is rendered on the frontend without any issues. I wonder if there's something wrong with my working copy? Hopefully an error like that would trigger some test failures but they seem to be in tact. |
|
And it fixed itself, super interesting! |
Co-authored-by: Adam Zielinski <[email protected]>
|
Before: After (with permalinks enabled): Two problems here - I don't know what the link is anymore, and after clicking edit the input is empty. |
|
It's fine to update the data model so that it tracks the ID instead of the URL, but the editor experience should remain the same. I don't like how I'm not able to preview (or edit) the URL anymore. Maybe we should fetch all the relevant page URLs once the editor is loaded? |
|
To enable that kind of editing we should fetch from the API details about the linked page. We don't know ahead of time which are the "relevant" pages so we'll end up with a spinner in that popover, so we can know the URL for the id. So it's still a downside. Also, when you edit the link, the link control will have to revert to "manual link" loosing the connection to the id. |
| { typeof value.id !== 'undefined' && ( | ||
| <span className="block-editor-link-control__search-item-header"> | ||
| { value && value.title && ( | ||
| <span className="block-editor-link-control__search-item-info"> | ||
| { value.title } | ||
| </span> | ||
| ) } | ||
| </span> | ||
| ) } |
There was a problem hiding this comment.
I think this part probably needs some design influence, would be good to address as a follow-up.
| } elseif ( isset( $attrs['url'] ) ) { | ||
| return esc_url( $attrs['url'] ); | ||
| } | ||
| return false; |
There was a problem hiding this comment.
false isn't a valid URL. Maybe just omit this?
| return false; |
| opensInNewTab, | ||
| title: label, |
There was a problem hiding this comment.
Hmm, these two aren't really the same thing. If I edit the label it doesn't mean that my page title is changed.
Not sure what the alternative is, since we don't actually know the page title.
| // End anchor tag content. | ||
|
|
||
| if ( $block->context['showSubmenuIcon'] && $has_submenu ) { | ||
| if ( isset( $block->context['showSubmenuIcon'] ) && $block->context['showSubmenuIcon'] && $has_submenu ) { |
There was a problem hiding this comment.
This change seems unrelated. Is it needed?
There was a problem hiding this comment.
It was a suggestion from @adamziel to make sure we don't get PHP Notices sometimes.
I think when you edit, the page title should be the value in the input box. Maybe. This is a tricky thing to get right. |
How I imagine it is this:
That's one way to approach it - it sounds fine to me, but maybe we could figure out even better UI here, for example disallow editing the page URL while still making it possible to edit the link URL:
^ This isn't the right solution, but my thinking goes in that direction. Also CC @shaunandrews since it's super relevant for in-toolbar editing. |
|
@draganescu no, but #23375 may. CC @shaunandrews |
Description
Closes #18345
How has this been tested?
Tested locally by:
Types of changes
Small change to
NavigationLinkEditto also save thetypewhichLinkControloffers to theonChangehandler.Then on the render function (PHP) added a call to
get_permalinkfor anything which is not an external URL.