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

Preserve the navigation on theme switch #38291

Closed
Tracked by #39324
adamziel opened this issue Jan 27, 2022 · 29 comments
Closed
Tracked by #39324

Preserve the navigation on theme switch #38291

adamziel opened this issue Jan 27, 2022 · 29 comments
Assignees
Labels
[Block] Navigation Affects the Navigation Block

Comments

@adamziel
Copy link
Contributor

adamziel commented Jan 27, 2022

Switching from Twenty Twenty-Two to another block theme resets the navigation.

Technically, it's because the navigation data is stored inside the template parts, for example:

<!-- wp:navigation {"layout":{"type":"flex","justifyContent":"right","orientation":"horizontal"}} -->
     <!-- wp:page-list /-->
<!-- wp:navigation /-->

The next theme is not aware of any customizations made and comes with its own presets. One theme I saw was preset to render a classic menu:

<!-- wp:navigation {"__unstableLocation":"primary"} -->
<!-- wp:navigation /-->

A different approach would be to infer the location/slug from the surrounding templates / template parts and use that to infer the new localization for the menu.

Prior discussions:


When this is implemented we should also consider the implications of #42600

@talldan
Copy link
Contributor

talldan commented Jan 31, 2022

Which would allow the next theme switch to seamlessly render the same navigation post with slug=primary as the previous theme.

I think tackling this is the key part.

The only feedback here is that I recall it was mentioned that the slug should be based on the template part area. Those areas typically use semantic names header, footer, sidebar, so the menus would have a slug like header-navigation.

The secondary part will be considering how that works for classic themes that switch to a block theme.

Another thing that would be on my mind is how WordPress will determine which menu should be surfaced for site navigation - #36667. Will it be via the slug? The two issues might have a dependency.

@mtias
Copy link
Member

mtias commented Jun 20, 2022

@adamziel a generic convention-based slug (primary, secondary, etc) was not the result from previous discussions; as @talldan points out, this is tied to the removal of the need to name a menu upon creation because we have enough context to create a meaningful semantic slug from it. There's an implied hierarchy of fallbacks to use on switches that should take into account the template and template part a menu is placed in. I don't recall if there was an issue capturing that.

@adamziel
Copy link
Contributor Author

@mtias I just updated the description.

There's an implied hierarchy of fallbacks to use on switches that should take into account the template and template part a menu is placed in. I don't recall if there was an issue capturing that.

AFAIR #36760 is the closest discussion. CC @draganescu @tellthemachines in case they remember something I don't.

@adamziel
Copy link
Contributor Author

adamziel commented Jun 29, 2022

@scruffian
Copy link
Contributor

scruffian commented Jun 29, 2022

@getdave @adamziel @draganescu and I spent some time thinking about this today. We came up with this suggestion:

Expected behaviour

  1. If the theme provides inner blocks to the navigation block, then always use the inner blocks. See question 2 below...
  2. If the theme has an empty navigation block, and the user has only one wp_navigation post, then the navigation block will automatically select this wp_navigation post (front and back end)
  3. If the theme has an empty navigation block and the user has multiple wp_navigation posts, then the navigation block will detect the wp_navigation post from any corresponding template parts.

Questions

  1. Is it reasonable to expect themes to provide an empty navigation block if they want to preserve the users navigation on theme switch?
  2. Should we treat the core/page-list inner block as a special case and also replace this with a user's menu so that the expected behaviour above works for navigation blocks with a core/page-list inner block? Most themes include this block by default.
  3. Should the frontend and the editor always display the same thing? At present the editor tries to infer what block you should use when you have only one wp_navigation post, but it doesn't save that change. This means that you see a different navigation on the front and back end until you save the change in the editor.

Technical details

In order to save the previous wp_navigation post in the new theme we need to either:

  1. Use a system of slugs to determine what wp_navigation is assigned to which location. We would need to update the slug if the user moves the navigation block in the block hierarchy, or updates menu location in the sidebar.
    [Try] menuSlug instead of navigationMenuId #36522

  2. Parse templates from the previous theme on theme switch, and then save the updated header in the database. We could find the template with a matching slug and find the first navigation block in this template, extract its ref and use this for the navigation block in the new theme.
    [Draft] Use template parts to preserve navigation between theme switches  #36117

cc @mtias

@mtias
Copy link
Member

mtias commented Jun 29, 2022

Themes cannot know what navigation structure a site has. We need to allow themes to be opinionated for cases where they want to have a very specific layout (where they'll basically use placeholder individual link-items) but otherwise defer to what exists on the site and the user expectation of how it should work. The latter is probably close to 99% of the cases we have. The situation of core/page-list needs to be broken down further because it's tricky:

page-list

Right now themes are using page-list to have a fallback, not because they explicitly want page lists to show instead of the user menu. This makes it difficult to disambiguate a genuine use of page-list (rare) from just a fallback use (most common). That also means changing behaviour can be rough because a user might have become accustomed to it showing all their pages even if they have an old menu somewhere. So without changing how things work today:

  • We can say themes that want to have page-list as a fallback should leave the navigation empty. When a navigation is empty we fallback to the most relevant menu, if there are no menus we internally use page-list.
    • We should be very clear with theme devs that adding inner blocks content on a navigation block is opinionated, whether that is page-list or individual links.
  • Editor and front-end should always be resolving to the same source.
  • Within the editor we have an opportunity to ask the user if a situation is ambiguous. We can render a modal when the user selects an unsaved navigation block that is using page-list and ask if they want to reuse a menu (when they do have a menu stored).

Technical details

Not sure I follow item 2. above. Why do we need to bother about the previous block theme? We should only look at wp_navigation (which should have a relevant slug) and wp_nav_menu to determine what to do.

@adamziel
Copy link
Contributor Author

adamziel commented Jun 29, 2022

@mtias item 2 is essentially doing the matching in a hook that gets executed during the theme switch. It alleviates the problem of preserving a meaningful slug throughout operations such as dragging a menu to a different template, choosing a different wp_navigation post for a menu that already has a slug etc. It comes with it's own problems, though.

@talldan
Copy link
Contributor

talldan commented Jun 29, 2022

Not sure I follow item 2. above. Why do we need to bother about the previous block theme? We should only look at wp_navigation (which should have a relevant slug) and wp_nav_menu to determine what to do.

My guess is because the slugs haven't been implemented yet. I think the block is still using ids. It does have the semantic title that could be used (slugified) until proper slugs are implemented down the line.

@mtias
Copy link
Member

mtias commented Jun 29, 2022

If we don't have semantic slugs for whatever reason we should just fallback to the next heuristic (I think we mentioned something like if only one wp_navigation exists, to use that, if there are many, to use the first retrieved, etc). It seems too convoluted and prone to mistakes to try to retro-add these on theme switch. There's a lot going on during theme switches and I'd rather not try to manipulate more things during it.

It's also worth keeping in mind slugs can be changed, so they can be an aid but we cannot rely on their presence all the time.

@getdave
Copy link
Contributor

getdave commented Jun 30, 2022

Thank you for providing some additional clarity here @mtias. There is a lot of discussion around the behaviour of the block at the moment, but I believe some solid planning at this stage will save as many hours of development time down the line so it's good to see the dialogue here.

Quick plea for fixing what we have

I would like to suggest that before we start implementing any of the more advanced "auto picking" of menus ideas, we should look to get what we have now working correctly.

As I see it that means making sure always have parity between what we see in the editor and what we have on the front end. This is inline with what you suggest above.

To that end I have a question about the implementation details around this goal.

The Goal

My understanding from what you communicated above, is that what we want to happen is for both editor and front end (i.e. PHP) to abide by the following rules:

  • if there are uncontrolled inner blocks present then render these and do not attempt any auto assignment of Nav block contents (i.e. the "opinionated" route you mention above - if Themes make this choice then they will forgo the "auto picking" of Nav Menus behaviour) (Update: see Respect uncontrolled inner blocks on Navigation block in editor and front of site #42182)
  • if no inner blocks are present then try to render items from any currently assigned wp_navigation post (assigned via ref attribute).
  • if that is not available then fallback to rendering the first wp_navigation post but only if only a single wp_navigation post exists.
  • if we don't have any wp_navigation posts or more than one exists, then fallback to rendering a list of pages.
  • if we don't have any pages then render nothing (of course in the Editor we would present some options to the user)

What happens for Editor fallbacks?

We can update the implementation to follow these rules. However, what happens in the case of the editor fallbacks when we get to the step of rendering a list of pages?

Currently on trunk, if there are:

  • no uncontrolled inner blocks
  • no assigned wp_navigation Post

...then block attempts to pick the first wp_navigation. If it cannot do that then it switches to "placeholder" mode and the user is offered a choice of:

  1. Create empty (i.e. start a new wp_navigation post).
  2. Choose existing - choose from existing menus (if any exist)

Screenshot 2022-06-30 at 20 05 08

However if we want parity with the front end then the editor would have to render a list of pages as it's placeholder. After all this is what the front end would do.

However in the editor we must still provide the user with choices. They must see the options to Create empty or Choose existing.

Therefore we're going to have to design/create a new placeholder state which is a hybrid of the current one and the page list block. Is that what you are envisaging?

For what it's worth I don't think we can simply auto insert an uncontrolled core/page-list block here because (as we saw in the "rules" above, uncontrolled inner blocks are considered sacrosanct. We would then have to give the page list block a special status which could become very confusing. I could be wrong however...

Example

For clarity, here is an example of how the editor and the front end are currently out of sync. The editor shows a placeholder and the frontend shows a list of Pages.

Screen.Capture.on.2022-06-30.at.20-04-37.mp4

@draganescu
Copy link
Contributor

However in the editor we must still provide the user with choices. They must see the options to Create empty or Choose existing.

Must we?

  • The page list block has an edit action.
  • The block itself behaves like the other blocks in terms of UI
  • There is an appender

... so what is a placeholder for? If pages are useless delete them and you get to a placeholder, which you created and nothing rendered on the front end. I think this state is missing in terms of sync:

  • as an editor when I delete everything inside a navigation block and save the template[Part]/page/post I expect to see nothing rendered on the front end

@getdave
Copy link
Contributor

getdave commented Jul 6, 2022

Must we?

I think at until we solve the auto-assignment of menus (including classic menus) we need to allow users to choose an existing menu.

Start from Empty should still be an option but perhaps it can be made less obvious because it's a 20% use case.

I'd still like to see a revised design for a new Page List based placeholder state. Perhaps @javierarce can help?

@javierarce
Copy link
Contributor

After chatting with @getdave and wrapping my head around this issue, I agree with @draganescu when he says that users have all they need to update one of these menus. If we can render a valid menu both on the front and the editor, then we don't need a special placeholder state. We only need one when users start from scratch or if we can't fall back to a valid menu.

@getdave
Copy link
Contributor

getdave commented Jul 21, 2022

Also related #42600

@getdave
Copy link
Contributor

getdave commented Jul 27, 2022

I've been looking into this some more and here's what I've learnt about how I think this will need to be implemented.

The Problem

Currently the Nav block has a ref attribute. This is a reference to the ID of a wp_navigation post which contains the "inner blocks" for the Navigation (i.e. the navigation items).

However IDs will change across different WP installations and thus using a post ID as the ref is not portable. For example, moving a Navigation block containing a ref attribute from a Staging environment to a Production environment will cause it to break because the wp_navigation represented by that ref will likely not exist on the Production environment. This could easily happen if you had a template part containing a Nav block which you moved from one environment to another.

The Solution

We need to use something to reference a wp_navigation post that can be consistent across environments.

One way to do this is to mirror what Template Parts does and use a string-based slug to identify the Post.

"slug": {
"type": "string"
},

We can then use this slug to look up the correct wp_navigation. The slug should be a string which is created by concatenating the slugs of each of the template parts (hierarchical) in which the Navigation block is contained.

For example the following hierarchy:

- header (template part)
-- header-small-dark (template part)
--- navigation (block)

...would result in the following slug: header-header-small-dark. We could opt to use an special syntax to denote where each "level" of the hierarchy begins such as header//header-small-dark or similar.

const getEntityArgs = [
'postType',
'wp_template_part',
templatePartId,
];

Data Fetching requires Post IDs

Many of Gutenberg's Core Data fetching utilities are optimised to expect numerical Post IDs. For example the signature of getEntityRecord expects:

  • kind (e.g. postType)
  • name(e.g. wp_navigation)
  • key (e.g. the Post ID)

If we drop the postId from the Navigation block in favour of a unique slug then it is not longer possible to pass the ID to these data functions. One thought might be that you can pass a query to the function:

getEntityRecord('postType', 'wp_navigation', { slug: 'header-header-dark-small' });

...however this will not work as the function does not accept a query (you must supply a key).

There is getEntityRecords plural which accepts a query as its 3rd parameter. It is possible to use this to retrieve the Navigation post by slug but it is inelegant.

getEntityRecord('postType', 'wp_navigation', { slug: 'header-header-dark-small' });

We may be able to work around this by altering the Navigation REST API endpoint to handle string-based IDs instead of numeric Post IDs. This concept is borrowed from the Template Part REST API endpoint which has special handling for just such a use case. Here templates can be requested by passing a string-based slug as the key:

getEntityRecord('postType', 'wp_template_part', 'twentytwentytwo//header')

You can try this in your devtools console by doing (replace the template part slug accordingly):

wp.data.select('core').getEntityRecord('postType', 'wp_template_part', 'twentytwentytwo//header')

Therefore for Navigation we could have a similar handling on the REST API to allow us to pass a string-based slug ref as key which would then return the correct wp_navigation post.

For example:

// does not work - yet!
getEntityRecord('postType', 'wp_navigation', 'header-header-dark-small');

...would result in the REST API querying for wp_navigation posts based on slug which is equal to header-header-dark-small.

Hierarchical Navigation Post Lookups based on Template Part Slug

We also need a way to look up the most suitable wp_navigation based on Template Part location.

Consider Theme A has a Nav block within the following template part hierarchy:

- header (template part)
-- header-small-dark (template part)
--- Navigation (block)

Theme B might have a different template hierarchy with only a single header template part:

- header (template part)
-- Navigation (block)

On Theme switch the following query will be issued. This is because Theme B's Navigation block will have no ref and thus will make a query for wp_navigation posts based on its template hierarchy position. As it is within the header template part it will issue the following query which will be empty:

getEntityRecord('postType', 'wp_navigation', 'header'); // null

However the API should be intelligent enough to know that the slug header represents a template hierarchy. Therefore it should now query partial matches for %%header%% which should result in the wp_navigation created in Theme A (which has the slug header-header-dark-small) being returned.

This mechanic (or something similar/better) would help to ensure that the most suitable Navigation post is returned for Navigation blocks blocks in similar template part hierarchies when switching Themes.

This has been partially explored in #36522.

Notes / Questions

  • What happens when a Navigation is created outside of a template part? For example in the Post Editor. How do we auto-generate the slug?
  • Can we rely on the Post handling to auto-generate unique slugs for blocks created within identical template part hierarchies. My experiments so far show that this results in deduplication via a numerical suffix:
    • header-header-small-dark
    • header-header-small-dark-2
    • header-header-small-dark-3
  • when creating a Navigation within a focused template part the editor doesn't know about the template part hierarchy and so an unexpected slug will be produced. For example, when you enter focus mode on header-small-dark and then create a Nav block the generated slug is header-small-dark, even though the Header Small Dark template part is enclosed within the Header (header) template part. This is due to focus mode. How do we work around this?
  • handling auto-drafts will need consideration.

@scruffian
Copy link
Contributor

There is getEntityRecords plural which accepts a query as its 3rd parameter. It is possible to use this to retrieve the Navigation post by slug but it is inelegant.

Why do you think this is inelegant?

@mtias
Copy link
Member

mtias commented Jul 27, 2022

What happens when a Navigation is created outside of a template part?

We should use the most immediate template, like index (main), archive, etc. The navigation block should not be available on the post editor.

results in deduplication via a numerical suffix

Yes, that's fine.

How do we work around this?

We should still be passing the entire context to focused mode when we have it since it's useful to display in different places.


It's also important to note that the idea of using the template hierarchy to set default names is also to aid in loading the right menu in the first place and not rely on things like "primary" — if there's a header menu we should try to prioritize it for any header area, etc.

@scruffian
Copy link
Contributor

The navigation block should not be available on the post editor.

At all? We should create an issue for this then.

@scruffian
Copy link
Contributor

However the API should be intelligent enough to know that the slug header represents a template hierarchy. Therefore it should now query partial matches for %%header%% which should result in the wp_navigation created in Theme A (which has the slug header-header-dark-small) being returned.

This sounds like a good approach. Alternatively we could just pull them all down and search though them, but it would be nicer to do it on the server.

@mtias
Copy link
Member

mtias commented Jul 28, 2022

At all? We should create an issue for this then.

It's not very important right now, but we have said theme building blocks should not be exposed in post-editor / post-content before, or at least not be very prominent.

@github-actions github-actions bot added the [Status] In Progress Tracking issues with work in progress label Jul 29, 2022
@mtias
Copy link
Member

mtias commented Sep 11, 2022

What's the status of this after the updates to what we load in the placeholder?

@getdave
Copy link
Contributor

getdave commented Sep 12, 2022

State of preserving Navigation on Theme switch for WP 6.1

The current status as I understand it (cc @draganescu and @scruffian for confidence check) is:

  • fallbacks for empty Navigation blocks are now the same across both front of site and editor - use the most recent Navigation Menu or use list of pages, via a page list block.
  • No need for Themes to include Page List block as default fallback as this is now inserted automatically if required.

For an 80% use case of a user who has a single Nav in the header and has a single Menu (or even a few different menus) the Theme switch experience is going to feel a lot more "seamless". In this case the block will pick the most recent Menu and display that in the block.

Where this is less optimal is for the 20% use cases where the user might have several Navigations and several Menus. In this cases it may well be that the Theme switch experience will require some manual "correction" by the user following the switch.

What we did not land in 6.1 is the ability to reference Navigation Menus by slug. This was intentionally deferred to 6.2 in order to allow more time for testing and refinement (especially as the feature offers little in the way of user experience when taken in isolation). This feature does however, unlock the door to allowing a more sophisticated theme switch experience, whereby the Navigation block will look to "intelligently" determine which Menu is most appropriate to use in the block (based on an as yet undetermined heuristic relating to template part hierarchy). I see this as a key feature of the block and so it's important we take the time to get it right.

I hope that helps?

@mtias
Copy link
Member

mtias commented Sep 14, 2022

Thanks for the overview, @getdave. Looks like it's in a much better place. What happens if I have a single classic menu and no new menus and switch to a block theme? Would it use page list or automatically import the classic menu?

@getdave
Copy link
Contributor

getdave commented Sep 14, 2022

ould it use page list or automatically import the classic menu?

Currently it would use page list.

@mtias
Copy link
Member

mtias commented Sep 14, 2022

That's a bummer, can we address the simplest case where it's 1 classic menu and no block menus so that it uses what exists? This should be much higher priority than sorting out the slug hierarchy. I'd also go as far as saying 1 menu || "primary" menu from classic should be used if you don't have block menus yet.

@getdave
Copy link
Contributor

getdave commented Sep 14, 2022

I agree. I believe @draganescu is already looking into it.

@scruffian
Copy link
Contributor

Here's an idea: #44173

@mtias
Copy link
Member

mtias commented Oct 7, 2022

Should we close this and keep remaining efforts on improving the slug allocation and resolvers?

@draganescu
Copy link
Contributor

#44173 solved the last part of this which is in the reduced scope here. Slug management is a larger scope.

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
Projects
None yet
Development

No branches or pull requests

8 participants