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

Navigation preservation plan #36087

Closed
draganescu opened this issue Oct 29, 2021 · 13 comments
Closed

Navigation preservation plan #36087

draganescu opened this issue Oct 29, 2021 · 13 comments
Labels
[Block] Navigation Affects the Navigation Block Developer Experience Ideas about improving block and theme developer experience [Feature] Themes Questions or issues with incorporating or styling blocks in a theme. Needs Technical Feedback Needs testing from a developer perspective. [Type] Technical Prototype Offers a technical exploration into an idea as an example of what's possible

Comments

@draganescu
Copy link
Contributor

draganescu commented Oct 29, 2021

Architecture for #35750

System* for navigation preservation on block theme switch using template parts:

  • to define navigation "areas" or "slots" or "locations" we use template parts, with the added convention that they're named navigation-*, where * is wildcard

  • in these navigation-* template parts themes can add navigation blocks with all attributes set, and inner blocks defined for default presentation and content

  • when the user changes the theme's default navigation content, for instance creates a menu, uses an existing menu or imports a classic menu, then the wrapping navigation-* template is updated and has to be saved, because the navigation block gets the new navigationPostId attribute

  • on saving the template part we hook into the saving flow and check if the slug of the template part is of the navigation-* form, and if it is:

    • we look for the 1st navigation block in the saving template part's contents
    • if found we collect the navigation block's navigationPostId attribute
    • we save somewhere in the DB (taxonomy, options, mods wherever) an association between the saving template part's slug and the collected navigationPostId attribute
  • on theme switch:

    • we look in the DB for the existing navigation-* template parts with navigationPostId associations (wherever we decided to store them)
    • we check if the theme has html template parts that correspond to the navigation-* associated slugs that we found in the DB
    • for every template part found of navigation-* form whose slug has a DB association to a navigationPostId
      • we get the new theme's navigation-* template part contents
      • we put into the contents the associated navigationPostId as an attribute of the first found navigation block
      • we save the new theme's navigation-* template part to the DB

This system has the following advantages:

  • we don't need to tap into the notion of areas for navigation, using something that exists
  • we reuse the fact that template parts with the same slug are automatically preserved on theme switch in new templates
  • we keep the navigation block independent of WordPress theme intricacies
  • we implement a server-side system that ensures a smooth transition between themes in terms of navigation preservation

This system has the following disadvantages:

  • it relies on a naming convention for navigation template parts
  • it introduces a special case for saving template parts
  • it relies on a lot of automagic on theme switch and makes some assumptions, some valid (one navigation block matters per navigation "area") some uncertain

To mitigate some of the disadvantages:

  • we could create a new block that works 99% the same like the template part block, so the saving of template parts is unaffected and we don't need the naming convention.
    • the downside here is that we'll have a new block visible in List View and as parent, and we've been in circles a lot around pass through blocks which never amounted to much
  • to avoid the automagic on theme switch we could tie the navigation block to the place where we store the association, have it know what the slug of it's parent template part is and get the navigationPostId from the DB
    • the downside is that a fundamental block is having some very special behaviour specific to themes

A reference implementation to follow.


*resulted from a discussion between @adamziel and @draganescu

@draganescu draganescu added [Type] Technical Prototype Offers a technical exploration into an idea as an example of what's possible Needs Technical Feedback Needs testing from a developer perspective. [Block] Navigation Affects the Navigation Block [Feature] Themes Questions or issues with incorporating or styling blocks in a theme. Developer Experience Ideas about improving block and theme developer experience labels Oct 29, 2021
@carlomanf
Copy link

Why not just mimic the way that menus work with PHP themes, and store the area identifier as an attribute of the navigation block? Why do template parts have to be involved?

@adamziel
Copy link
Contributor

adamziel commented Oct 30, 2021

@carlomanf It was explored by @getdave in #36002. Storing that identifier with the block creates a scenario where you have two different data sources: there’s the custom post type with navigation items, and there’s the area identifier. Which one wins? What happens if I change the area from „primary” to „secondary”? What happens if I customize menu items?

My thinking is that it’s weird because the „slot” (/area) and the „menu” are really two different concepts. A theme may specify two slots, but I may have the same menu in both, two different menus, three menus in total, etc. Template parts are handy in that they provide all the „slot” mechanics required to model that.

@carlomanf
Copy link

carlomanf commented Oct 30, 2021

I haven't seen that branch, but is it possibly a similar conundrum to what I described at the end of this comment?

For example, you have a navigation with post ID 101 and area "primary," and another navigation with post ID 102 and area "secondary." Then you have a navigation block instance with attribute "primary" and you want to change the block to render the other navigation. How should this be done?

  1. change the block attribute from "primary" to "secondary?"
  2. change the "primary" navigation from 101 to 102?
  3. create a brand new area identifier and assign 102 to that area?

If I'm reading the conundrum correctly, then I would say it will be tricky to get the user experience right, but at least it has no technical limitations. The template part approach proposed here has a few limitations, one being that it won't work for navigations included directly in a template, and another being that it forces the template part identifier to have the "navigation" prefix.

@talldan
Copy link
Contributor

talldan commented Nov 1, 2021

It was explored by @getdave in #36002. Storing that identifier with the block creates a scenario where you have two different data sources: there’s the custom post type with navigation items, and there’s the area identifier. Which one wins? What happens if I change the area from „primary” to „secondary”? What happens if I customize menu items?

My thinking is that it’s weird because the „slot” (/area) and the „menu” are really two different concepts. A theme may specify two slots, but I may have the same menu in both, two different menus, three menus in total, etc. Template parts are handy in that they provide all the „slot” mechanics required to model that.

I don't really see this as much of an issue:

  • Navigation block with an 'area' - The block cannot have a menu id associated with it, only an area. Selecting a different menu changes the menu assigned to the current area.
  • Navigation block without an 'area' - The block doesn't have an area associated with it, only a menu id. Selecting a different menu updates the menu id associated with the block.

Is this something we tried? We are in full control over the navigation block's UI, so if it needs to look different when an area is assigned, that's completely possible.

@adamziel
Copy link
Contributor

adamziel commented Nov 1, 2021

The template part approach proposed here has a few limitations, one being that it won't work for navigations included directly in a template

@carlomanf navigations included directly in a template are equivalent to navigations with area = none from Dave's implementation. I don't think of it as of a limitation, as much as a way of having both "labeled" navigations and "unlabeled" navigations. How do you see it?

and another being that it forces the template part identifier to have the "navigation" prefix.

I agree this isn't ideal, although I don't think it's terrible either. I experimented with using template areas instead in #36117. It would also be easy to add a new free-text field such as "is_navigation" or "type" to theme.json.

@adamziel
Copy link
Contributor

adamziel commented Nov 1, 2021

I drafted that idea in #36117

@talldan I want to avoid yet another implementation of reusable content. That's the entire reason why I'm proposing an approach based on the existing primitives.

We already have reusable blocks, template parts, options-backed blocks like site logo, and now we're talking about navigation areas. The experience already tends to be confusing for me, and I use it every day. I imagine it's even more confusing for the regular user. It's hard enough to make a simple popup work consistently across the regular links and navigation links. I'd rather avoid running into that in the "reusable content" world.

I think the source of trickyness is that the "navigation area" model is, essentially, a one-to-many relationship with three parts:

  1. Navigation areas in the theme
  2. Navigation posts in the database
  3. The connection between the two via a label such as "primary", "secondary", "none", etc.

To manage the data effectively, you need to have some UI for each part. Posts are displayed as Navigation Blocks. Areas are not displayed right now, besides the "area" dropdown in the block inspector. As @getdave noted:

The currrent UX of having a single dropdown on the Nav block itself for "Navigation area" does allow us to handle these permutations. We're still stuck in that there is now no way to assign/reassign/unassign a wp_navigation post's association with an area.

Adding an "area" attribute to the navigation block = managing this "one-to-many" relationship via the block's inspector. I don't think it's a right place for such an important piece of UI. It would have to be a new control or a modal, which would disconnect it from the visual experience provided by Gutenberg.

I believe all these problems were already solved: One-to-many relationships are easily managed through template parts. I can create a template part and use it in multiple places. Applying that to menus: If I want a different menu in just one place, I can choose a different template part or even have a "one-off" menu not wrapped in any template part. If I want to change the "primary" menu everywhere, I do the same familiar thing as when I want to change the header everywhere. I want that to be the same experience because I heavily agree with what @jasmussen said:

As I read the PR and tested the various steps, I felt that the goal being sought was one that could benefit every block area, not just the contents of the navigation.

Staying in the "template part" world is IMHO a good forcing function to discover any shortcomings they have. All the improvements would move the larger FSE project forward. Rolling out a custom implementation, however, would create a new pocket of expertise somewhat separate from the bigger picture.


Also, tactically:

Navigation block with an 'area' - The block cannot have a menu id associated with it, only an area. Selecting a different menu changes the menu assigned to the current area.
Navigation block without an 'area' - The block doesn't have an area associated with it, only a menu id. Selecting a different menu updates the menu id associated with the block.

This restriction could work, but I'd much rather have a data model where an invalid state can't be expressed in the first place. I fear that, sooner or later, we'd end up with blocks that somehow got both attributes set.

@draganescu
Copy link
Contributor Author

One thing I thought is that using template parts removes the need for a "special" isolated editor as well, because now we always have the navigation block wrapper :)

@talldan
Copy link
Contributor

talldan commented Nov 2, 2021

Ok. I'd encourage exploration around this. I think my concerns would be a more complex user experience (there are now two blocks instead of one), and also the complexity of theme switching has increased (we're explicitly re-writing content, while the other proposal should just work based on matching template part area names, an already well defined semantic).

There are some possibilities for invalid state here too. Like if I wrap my navigation block with another template part in the editor, the navigation block will no longer be in my special navigation template area and theme switching breaks.

Another downside is that some block attributes specific to a theme (like colors, custom block styles, and other style attributes) are saved and transferred to the new theme where they're potentially meaningless or look out of place. This advantage of keeping visual style separate from menu data is no longer as much of an advantage.

I don't want to dissuade though, just provide some constructive feedback, maybe these are things you already have ideas for, they just haven't been made visible in the discussion yet.

@adamziel
Copy link
Contributor

adamziel commented Nov 2, 2021

there are now two blocks instead of one

I agree it's more complex than having just a single block. At the same time I like it because in my mental model there are two entities there (navigation area and navigation post), so this at least makes that visible. My question now is "how should that work for the user?"

the complexity of theme switching has increased (we're explicitly re-writing content, while the other proposal should just work based on matching template part area names, an already well defined semantic).

It's true that re-writing content adds complexity. At the same time, I understand the other proposal introduces a new concept of "navigation areas" and does not rely on the existing template areas semantics. I like the idea of matching the names, though. I'm exploring a related idea that combines both approaches and doesn't involve re-writing content on theme switch.

if I wrap my navigation block with another template part in the editor, the navigation block will no longer be in my special navigation template area and theme switching breaks.

Do you mean a structure like below?

| Template part (Header)
+--| Template part (Primary menu)
.....+--| Template part (One more layer)
..........+--| Navigation block

Agreed, that would be a problem. It would be nice to only accept navigation blocks in that navigation template part. Or at least don't accept nested template parts. Maybe that could be a new flavor of the template part block.

Another downside is that some block attributes specific to a theme (like colors, custom block styles, and other style attributes) are saved and transferred to the new theme where they're potentially meaningless or look out of place.

I don't follow – the idea is to only rewrite navigationMenuId and nothing else. The PR does not transfer colors or custom styles. I may be missing something, would you please elaborate on that?

I don't want to dissuade though, just provide some constructive feedback, maybe these are things you already have ideas for, they just haven't been made visible in the discussion yet.

Much appreciated @talldan! It's helpful, to the point, and helps me direct these explorations. Please keep it coming.

@adamziel
Copy link
Contributor

adamziel commented Nov 2, 2021

@talldan I added a new idea to #36117:

In that implementation, we'd still have template areas such as primary-menu. Themes would ship default template parts like:

<!-- wp:navigation { "initialNavigationMenuArea": "primary-menu" } -->

When such template part is saved, {"primary-menu": "<navigationMenuId>"} is stored in the database. Rendering relies on navigationMenuId, but when it's missing it falls back to initialNavigationMenuArea which reads the stored <navigationMenuId> from the database.

This way we:

  • Lean on keyword matching for populating menus in new themes.
  • Lean on template parts for all the things UI and UX. If there are any problems there, they may be opportunities to improve the larger template parts experience. If they're not, it could indicate this idea isn't quite right.
  • Have menus that behave in the same way as headers, footers, and other template parts on theme switch. It's all a single, consistent experience.

The only remaining problem I see here is having an extra template part inside primary-menu. I think it can be either solved by restricting allowedBlocks, or documented as unsupported for now. Did I miss anything? What do you think? Also cc @draganescu

@talldan
Copy link
Contributor

talldan commented Nov 3, 2021

I don't follow – the idea is to only rewrite navigationMenuId and nothing else. The PR does not transfer colors or custom styles. I may be missing something, would you please elaborate on that?

Yep, so previously any customisations to a navigation block that might be specific to a theme were saved in a template part for that theme. For example a header template part for a particular theme might have showSubmenuIcon set to false and item alignments set to right.

When switching to a new theme, my new theme would have a different header template part, and it might have showSubmenuIcon set to true and central alignment because that's what the theme creator intended.

After this change, all the navigation block attributes are migrated and will be active after a theme switch because they're retained in the new template part. So after switching I'd still have showSubmenuIcon set to false and alignments set to right, which goes against what the author of my new theme intended. It might look incorrect after switching.

Looks like I'm wrong about this, only the navigationMenuId is migrated from the old theme.

@talldan
Copy link
Contributor

talldan commented Nov 3, 2021

At the same time I like it because in my mental model there are two entities there (navigation area and navigation post), so this at least makes that visible. My question now is "how should that work for the user?"

Yes, this is a challenge. Possibly the easiest flow would be to add a button to the navigation block itself that when clicked wraps the nav block with the template part (titled something like 'Define as navigation area').

Also have to consider flows like pattern insertion, because that was flagged in #35947 as already being more difficult with the current version of the block. I think the right direction would be to encourage pattern developers to already have the navigation area within patterns where it makes sense.

Agreed, that would be a problem. It would be nice to only accept navigation blocks in that navigation template part. Or at least don't accept nested template parts. Maybe that could be a new flavor of the template part block.

A dedicated navigation area block may be the most simple option. It could have templated inner blocks to ensure it only has a direct navigation block child that can't be removed.

I was also wondering whether there might be a way to avoid needing to modify the template part content on theme switch. I had a vague idea:

  • The navigation menu id could be stored in the template part (via something like post meta)
  • In the editor, a navigation area block could provide the navigation menu id to the nav block via block context.
  • When switching menu in the navigation block, the parent block's attributes would have to be updated instead of the nav block itself. This is the only bit I don't like as much, but it isn't difficult to achieve.

@getdave
Copy link
Contributor

getdave commented Nov 8, 2021

Is this now able to be closed out?

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 Developer Experience Ideas about improving block and theme developer experience [Feature] Themes Questions or issues with incorporating or styling blocks in a theme. Needs Technical Feedback Needs testing from a developer perspective. [Type] Technical Prototype Offers a technical exploration into an idea as an example of what's possible
Projects
None yet
Development

No branches or pull requests

5 participants