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 / Page List: Allow converting to individual menu items #29437

Closed
jasmussen opened this issue Mar 1, 2021 · 14 comments · Fixed by #30390
Closed

Navigation / Page List: Allow converting to individual menu items #29437

jasmussen opened this issue Mar 1, 2021 · 14 comments · Fixed by #30390
Assignees
Labels
[Block] Navigation Affects the Navigation Block [Block] Page List Affects the Page List Block [Status] In Progress Tracking issues with work in progress

Comments

@jasmussen
Copy link
Contributor

What problem does this address?

When you create a navigation menu and press "Add all pages", you get the Page List preinserted. This block stays in sync with pages you create or delete on your website, and is likely a good default experience for most people.

Every once in a while, you need a more curated experience:

  • You want to delete a page
  • Edit the label of a page link
  • Change the order of pages

What is your proposed solution?

Allow the Page List block to be converted to single menu items, so those can be reordered, deleted or tweaked at will. Here's a mockup:

Convert to editable

  • Select the Page List block
  • Click "Edit" in the toolbar
  • The modal confirm prompt is an opportunity to explain what the transformation does.

The phrasing of the text in the modal could use fine tuning. It needs to be brief, simple, to the point, but still explain the purpose.

@jasmussen jasmussen added Needs Copy Review Needs review of user-facing copy (language, phrasing) [Block] Navigation Affects the Navigation Block [Block] Page List Affects the Page List Block labels Mar 1, 2021
@kristastevens
Copy link

Hi @jasmussen, great to run into you! Here's some suggestions for the copy. Let me know if you have any questions or need some clarification.


To edit this menu, convert it to single menu items. This allows you to add, re-order, remove items, or edit their labels.

Note: if you add new pages to your site, you'll need to add them to your navigation menu.

CANCEL CONVERT


@gwwar gwwar assigned gwwar and unassigned gwwar Mar 1, 2021
@jasmussen
Copy link
Contributor Author

Thank you Krista, it's always a pleasure! This is great. 👌

@jasmussen jasmussen removed the Needs Copy Review Needs review of user-facing copy (language, phrasing) label Mar 2, 2021
@talldan
Copy link
Contributor

talldan commented Mar 5, 2021

It could be worth using the term 'page links' instead of 'menu items'. Menu items is what the old system is called, and Page Link is the block name that it would be converted to.

@jasmussen
Copy link
Contributor Author

jasmussen commented Mar 5, 2021

Great point. Here's an update:

Screenshot 2021-03-05 at 10 18 44

@talldan
Copy link
Contributor

talldan commented Mar 5, 2021

Also wondering if this dialog could be something declarative that's built in to the transform system for blocks. It could be useful for transforms where there's potential for content to be removed (for example the Cover to Image transform).

I'm assuming here that converting Page List -> Page Links would be implemented as a transform under the hood.

@jasmussen
Copy link
Contributor Author

It would be nice with a systematized way of being able to show a dialog on transforms like these. I could see it benefitting the classic block > blocks conversion also, even though that's probably a different type of conversion.

I'm assuming here that converting Page List -> Page Links would be implemented as a transform under the hood.

Presumably. Should there also be a way to transform one or more menu items into a Page List? I.e. select all menu items and convert to page list? It would probably be a lossy transformation as they'd very probably be out of sync.

@gwwar
Copy link
Contributor

gwwar commented Mar 23, 2021

I can explore this one, @talldan I believe this will be one of the first dynamic blocks to be transformed?

For example, HTML->Convert to Blocks is handled in packages/blocks/src/api/raw-handling/index.js while the block transform API also takes a client JS function https://developer.wordpress.org/block-editor/reference-guides/block-api/block-transforms/.

@gwwar gwwar self-assigned this Mar 23, 2021
@talldan
Copy link
Contributor

talldan commented Mar 24, 2021

@talldan I believe this will be one of the first dynamic blocks to be transformed?

Hadn't thought of that. You're right, there don't seem to be any transforms for dynamic core blocks.

I guess this will be a bit more of a challenge.

@gwwar
Copy link
Contributor

gwwar commented Mar 24, 2021

I ended up bugfixing today, but some early thoughts:

Solving for the General Case

Are there any other core dynamic blocks that need information to transform in the "to" direction?

We need page link information

For this particular use case, block attributes are empty. So the client transform has no information.

Some quick ideas:

  • Pass through extra information in register_block_type_from_metadata ?
  • Multiple API fetches for pages using existing endpoints (Not my first pick since other dynamic blocks might require misc information)
  • New transform server callback. Block editor can replace contents from the callback.
  • Maybe parse the raw block render?

Screen Shot 2021-03-24 at 4 07 20 PM

Inserting the list of links

Even if we have all information to create the links in a to transform, we'll need sibling insertion of those links in the parent navigation block.

Assuming that page-list contains "Test" and "About"

<!-- wp:navigation {"orientation":"horizontal"} -->
<!-- wp:navigation-link {"label":"Page List","type":"post","id":683,"url":"http://wordpress.local/page-list/"} /-->
<!-- wp:page-list /-->
<!-- wp:navigation-link {"label":"Drag Test","type":"post","id":667,"url":"http://wordpress.local/drag-test/"} /-->
<!-- /wp:navigation -->

With the current transformation APIs it isn't possible to generate something like:

<!-- wp:navigation {"orientation":"horizontal"} -->
<!-- wp:navigation-link {"label":"Page List","type":"post","id":683,"url":"http://wordpress.local/page-list/"} /-->
<!-- wp:navigation-link {"label":"Test","type":"page","id":229,"url":"http://wordpress.local/test/"} /-->
<!-- wp:navigation-link {"label":"About","type":"page","id":34,"url":"http://wordpress.local/about/"} /-->
<!-- wp:navigation-link {"label":"Drag Test","type":"post","id":667,"url":"http://wordpress.local/drag-test/"} /-->
<!-- /wp:navigation -->

The block transform API expects <!-- wp:page-list /--> to be fully replaced with a single block. While we can create blocks with innerBlocks, we also want to avoid nesting two navigation blocks. Let me know if any of my assumptions here are incorrect.

This shouldn't be too bad to update in the block transform API, but I think it's worth verifying that there are more use cases to support this. If it's already possible to do this, let me know 👍

In case y'all were interested cc @gziolo @youknowriad @talldan @ellatrix

@talldan
Copy link
Contributor

talldan commented Mar 25, 2021

Are there any other core dynamic blocks that need information to transform in the "to" direction?

I can't think of any. Something like Archives/Latest Posts to Post Link seems like the closest thing, but the blocks aren't useable in the same context. A lot of the existing dynamic blocks were created as replacements for widgets, so don't necessarily map to other blocks.

We need page link information

The server side transform sounds like the most plausible. What I have in mind is:

  • Server side, the block has a transform declaration very similar to the client side transform configuration.
  • These transforms are merged into the client side list but marked as a server side transform during this process.
  • When a server side transform is matched, the client calls a REST API endpoint (GET /block/transform?) to get the block conversion data instead of the normal callback function.

Considerations:

  • I don't think it'd be easy to get the isMatch function working for such a server side transform, though this probably isn't a concern for this particular implementation.

Another option could be to change how Page List renders in the editor—move it away from using ServerSideRender, so that there is actually data client side (though it'd still be challenging to get this into the transform callback). There was some discussion about the editor implementation during the original PR along with the idea to make Page List render Page Links internally.

Even if we have all information to create the links in a to transform, we'll need sibling insertion of those links in the parent navigation block.

I think this bit is possible. Gallery has a similar transform to images. Looks like returning an array of blocks is the way to achieve this:

if ( images.length > 0 ) {
return images.map( ( { url, alt, caption }, index ) =>
createBlock( 'core/image', {
id: ids[ index ],
url,
alt,
caption,
align,
sizeSlug,
} )
);
}

@gwwar
Copy link
Contributor

gwwar commented Mar 29, 2021

One idea @dmsnell had was to refactor the page list block to save navigation links as inner blocks (instead of producing the custom markup we generate), which would allow us to use the existing block transform API.

Page lists are currently embeddable outside of the Navigation block so there are some corner cases to consider, but it might be worth exploring.

@mcsf
Copy link
Contributor

mcsf commented Mar 30, 2021

👋 — I think this is specific enough that we shouldn't be generalising around a new API yet. Some notes from my train of thought:

  • Throughout this issue we quickly started to discuss the idea of transforming a block, but Joen's original description and mockups do show a block-specific interaction in the form of an Edit button in the block's own toolbar. To me, this is consistent with how an Image block displays a Replace button. Another good data point from the Image block is that there is a block-specific button labeled Add text over image, which does trigger a transformation in Gutenberg's formal sense, but hides all that from the user. The result matters, not the juggling of types. The same approach fits this issue.
  • If a Page List block sits inside a Navigation block, we have room to explore different perspectives: should Page List manage its own transformation and assimilation among its siblings, or is the parent in a better position to do so? (Maybe not, but the question is necessary.)
  • Even though I don't necessarily block this issue, yes, I agree that it's healthy to ask the question once again about whether to move Page List away from ServerSideRender.
  • I looked at the different patches — Blocks: add experimental dynamic block transform endpoint #30358, [wordpress-develop#1138](Blocks: add transform_callback for dynamic blocks wordpress-develop#1138 — and, although the server-side implementation of transform_callback seems in itself correct and consistent with its surroundings, I believe adding transform_callback actually muddies our Transforms API. My main reasoning is that, on the server, as you know, one never operates on the actual in-memory block (the thing returned and managed by @wordpress/blocks, the only component that has full knowledge of a block), but only on the raw parsed form as returned by parse_blocks. In the case of render_callback, this was a concession we made very early on because the need for dynamic rendering was too great and less dependent on having full server-side parsing, but I wouldn't make the same compromise for transforms unless we really have to.

Can we then explore a solution that doesn't introduce a new API? Keeping this specific to the Page List and/or Navigation should give enough flexibility for the blocks to do whatever juggling data is necessary, coordinating REST API requests with editor APIs (replaceBlocks, etc.).

If I've overlooked something crucial, please call me out.

@gwwar
Copy link
Contributor

gwwar commented Mar 30, 2021

Thanks for the feedback @mcsf you caught me a bit early! From the quick sketches in #30358 WordPress/wordpress-develop#1138 a few things stand out:

  • The block transformation APIs expect transforms to be relatively quick. Supporting any async transforms will require a lot of API surface area to update. So this approach isn't particularly viable.
  • I didn't really suss out folks having a need to transform other dynamic blocks yet which is another point to pause on this approach.
  • There were a few use cases of wanting to build up blocks on the server. A helper like create_blocks might be useful. I saw a few spots where we mimicked output of parse_blocks
  • I need to explore this, but it may be interesting to allow render_callback to return blocks.

Can we then explore a solution that doesn't introduce a new API?

Yes, I was going to look into seeing if a PageList refactor is viable. The markup that this particular block generates isn't particularly interesting, so we could simplify things by saving navigation links to the innerBlocks. The deprecation path might need some work, as page list can exist outside of navigation blocks.

Another area to explore is whether PageList is still needed as a block. There are a few odd combinations the current implementation allows. It feels more like a mode of the Navigation block container if it's controlled or if the user may add arbitrary links.

@mcsf
Copy link
Contributor

mcsf commented Mar 30, 2021

Thanks for taking the time to assess this fully!

Yes, I was going to look into seeing if a PageList refactor is viable. The markup that this particular block generates isn't particularly interesting, so we could simplify things by saving navigation links to the innerBlocks. The deprecation path might need some work, as page list can exist outside of navigation blocks.

Another area to explore is whether PageList is still needed as a block. There are a few odd combinations the current implementation allows. It feels more like a mode of the Navigation block container if it's controlled or if the user may add arbitrary links.

This is good to look into. Another thing comes to mind: the Query block has more in common with Navigation and Page List than one might think. In order for Query to work, we needed to be able to reconcile a few things; in particular, a Query block is capable of "containing" dynamic things. In its serialised form, a Query block will contain Query-related blocks, like Query Title, etc. But in the editor, once this all materialises as living blocks, the Query block contains posts, each of them containing a title, etc. This isn't necessarily the best path for Navigation, but it's food for thought on the whole question of dynamic vs. static. :)

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 [Block] Page List Affects the Page List Block [Status] In Progress Tracking issues with work in progress
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants