Add basic pages block #28265
Add basic pages block #28265
Conversation
|
Size Change: +3.79 kB (0%) Total Size: 1.37 MB
ℹ️ View Unchanged
|
|
Thanks so much for working on this! |
|
Very cool to see this in the works! Here's what I see when the Pages block lives alone:
And here's how it looks when placed inside the Navigation block:
|
|
I've got most of the styles for displaying submenus as dropdowns in the nav block, but dropdown background colour isn't working yet because Pages isn't inheriting the styles from the parent yet. Submenu icons is also not wired to the parent yet - but I'm not sure it makes sense to have it as a setting in the standalone Pages block, unless we provide the option to use dropdowns there, too. The problem is I can't think of a way to render the icons in Pages unless that exists as a setting 🤔 |
Can the pages block inherit whatever property is set on the Navigation Container? I.e. if the pages block is inside the navigation block, and the navigation block has the option toggled on, so does the Pages block? |
tellthemachines
requested review from
ajitbohra,
spacedmonkey and
TimothyBJacobs
as code owners
tellthemachines
force-pushed
the
add/pages-block
branch
from
2ca3697
to
10e32af
Compare
|
This is so good. So so good. Thank you for working on this. Here's what I see:
That's the Pages block inside navigation. I then proceeded to create a new page, "Journal", and sure enough, it shows up automatically when I reload:
This is the promise of the block itself, and it feels totally validated by this. I need to do some deeper testing, specifically with regards to how subpages work. But just as an initial test, this is very promising. Some small things:
Can we make it so when you press "Add all pages", it adds just this one block, as opposed to all the individual menu links?
We should probably rename this one from "Pages" to something else. "Page Listing" was suggested, that might be worth a try. I'd also like to provide you with a new icon for this one, I shall return. Finally, something is making the markup and visual appearance differ from individual menu items, compared to items from that page listing. I have a black underline in my theme that shows up for the page listing (see above), but it's gone for individual items:
I'd like to dive in and see what that might be and how I can potentially fix it. It's probably a separate thing regardless, but thought I should mention it. The vertical menu still shows up as horizontal:
I can also click on the menu items:
We can probably make the menu items inactive using CSS, I was able to select the page listing block using the block navigator:
How hard would it be to add a button in the page listing block toolbar that says "Convert to individual menu items"? (Or a shorter phrase if we can think of one) Thanks again for your work 🏅 |
|
Working really nicely so far. I'd worry that the heavy reliance on server side rendering and the walker might end up being a limiting factor in future changes, like if we want to change the markup. The alternative (making it work more like the query block does) would be pretty complicated though, so might be best to go for the simple solution first and see how well it works (which I think is pretty successful so far). I echo most of what was said in the other feedback. TT1-Blocks has a couple of issues (pages always appear vertically, and submenus don't open). In TT1 normal, the submenus are a little narrow for longer menu names: |
Done ✅
Could it be "Page list" instead? "Listing" sounds more like a single listed item, so may be confusing.
In the editor, it's the default browser
Fixed, and fixed!
I thought initially that it would be simple to transform a Pages block into a Links block once #24644 is done, but given that Pages should work as a standalone block, would we want that functionality to also apply when it isn't part of a Navigation? We don't have any way of conditionally rendering controls depending on the parent block (I think - because blocks aren't supposed to be aware of their parents) so not sure how we'd go about making it Navigation specific.
I'm more worried both about the performance impact (which is hard to tell now because query only renders 10 items at a time - unless I'm missing something) and the duplication of markup (query loop has its
Hmm, I can't reproduce that by adding a Nav block with a Pages block inside the header in the site editor 🤔 |
Go for it!
Thanks for the investigative work. Seems like it would be nice if we were able to change those contenteditable divs to contenteditable
Definitely best to try and avoid making anything Navigation specific if we can. But "navigation aware" might not be the end of the world if it's done in a clean way. Specifically, I'm not sure that this Pages (or Page List) block has to work outside of the navigation block. I think that's a really nice benefit. And yes, I think it would be totally fine to have that functionality also work when used on its own. I compare it to a gallery of 5 images, which you can transform to 5 individual image blocks. (I realize here the transformation from Page List to individual Paragraphs with links inside them would be an irreversible transformation, but I don't see that necessarily as a problem). Edit: I'm just realizing that they'd need to transform to Link blocks, which to your point probably wouldn't work outside the nav block? :thinkspin: |
This might not be an issue, I don't think the transform system will show a transform to a block that can't be inserted. Worth double-checking, but an example is that you can't transform a Nav Link to a Group because Group isn't allowed in Navigation. |
|
I am so excited for this PR to land, it really improves things. Instead of the "page" icon you're using for the new block, I created a new icon you can add as "pages":
Here's the SVG: Let me know if you'd like me to push directly to your branch to add this. |
|
This one fixes #16635. |
|
Ok, I think I've addressed all the feedback, so if everything is now OK I'll move onto adding some tests for this block. The transform from Page List to Links is dependent on #24644, so let's do it as a separate PR later. |
|
So excited for this one, thanks again. I'm happy to push the icon change separately. A quick question just to be sure — you mentioned how on the frontend and the page list, the links are links ( The consistency in markup between menu items would really simplify some of the CSS! |
Whoops, I made the change and then forgot to push it 😳
Let's try that as a separate task! I can't think offhand of reasons not to do it, but might be missing something. |
|
I've added the tests, so this is ready for a final review now! |
Awesome. Want me to ticket it? I'm seeing jump when the page list loads:
You can fix that with this CSS: Then it looks like this:
I noticed submenus have no background:
This is because the markup for the child blocks is this: This is compared to the markup of "classic" navigation submenus: I know there's a goal to make this Page List block work outside of the navigation block. Can it be aware of its parent, and show the navigation-block appropriate CSS classes when it's used there? Finally, I'm really missing that feature to convert a Page List block into menu items. Whether that's a transformation, or a button in the toolbar, "Convert to menu items", it's important that we land this feature. Doesn't necessarily have to be part of this PR specifically, but we should have some confidence that it's possible to do, so we can follow up with it. Those were the three things I found. But the primary take-away: this is feeling so good, and it even feels sort of obvious and predictable. I think it will solve a bunch of headaches, thank you again for working on this! |
|
Adding the new block in the navigation block produced this result on Edge: Front end:
Back end:
Also clicking on the page items navigates to the page, which is different from the behavior of the other links in paragraphs or navigation menus, that don't navigate. With children the styling is still different, is it intentional?
From the PR's description it looks like this is expected since I guess the differences in markup make this block appear different. Will a solution to #24644 / #24364. also fix these visual inconsistencies? |
There was a problem hiding this comment.
Others have done a fair amount of testing, so this review focuses mostly on code. Looks good so far.
I'm also still not sure about using the walker approach, we can always try different things once shipped if it feels like it's not working for us. I think that's one of the main reasons we should keep it behind a the GUTENBERG_PHASE flag for now (although I think it'll miss 5.7 anyway).
| clientId, | ||
| 'core/navigation' | ||
| )[ 0 ]; | ||
| return getBlockAttributes( parentBlock ); |
There was a problem hiding this comment.
I think we should be able to use block context instead of selecting from the navigation block's attributes via the block-editor store.
The good thing is it's a bit easier, it's just passed in as a prop:
https://developer.wordpress.org/block-editor/developers/block-api/block-context/#using-block-context
Looks like we should also make this change to nav link at some point as well.
| .wp-block-page-list { | ||
| background-color: inherit; | ||
| } | ||
| // Make links unclickable in the editor |
There was a problem hiding this comment.
I think the <Disabled> component was the previous way of doing this (see the Archives block). Not sure what the pros/cons are of the two ways.
There was a problem hiding this comment.
Hmm, that won't work here because Disabled will prevent any dropdowns from opening. It works for Archives because there are no nesting levels there. It uses the same CSS to achieve the purpose though.
packages/e2e-tests/specs/experiments/__snapshots__/navigation-editor.test.js.snap
Show resolved
Hide resolved
That makes sense! I'm going to explore another way of rendering the markup server-side which will allow us more flexibility. Potentially we could use the "show submenu icon" context to ascertain whether the block is inside a Navigation block, though it feels slightly hacky to do so 😬 🤔
Probably not in this case, because Page list doesn't have inner blocks. Its contents are not supposed to be editable, so we're generating the markup for the dropdowns server-side. That doesn't mean the classnames can't match though. I'm thinking perhaps we could change the classname we're hooking the dropdown styles to in the Navigation block to something more generic, e.g. instead of (
Yeah, "Transform to Links" is waiting on #24644, so best do it as a separate task. We can do a follow-up PR to add all the transforms. |
I believe that we will be seeing a lot more of this blocks being aware of their containers in the future. For example a task block could become a task list when inside a list block. Or perhaps more imminently, a nested Image block inside a Gallery block could gain new powers of awareness of parent properties such as "Open images in new tabs". CC: @glendaviesnz in case he has input here. |
|
Alright, I got rid of the walker and we're now iterating through the pages and building the nested list manually, so it's easier to customise the markup to our hearts' content. But we're not able to use parent block context to conditionally render markup yet: it works on the front end, but isn't recognised when using I'll write up an issue so we can look into the best way of doing this. I'm not convinced that client-side rendering in the editor is in our best interest, as there's a noticeable performance degradation compared to server-side rendering. In the meantime, I think we've exhausted the limits of what can be done in this PR. Final reviews welcome! |
There was a problem hiding this comment.
Blitz those final things and I reckon this can be merged.
Great job with the refactor away from the walker. I really like this block and think it'll probably be how a lot of people build menus, given how it just automatically works.
Any thoughts on next steps? Probably filtering the pages (category/tag) would be useful.
| static $block_id = 0; | ||
| $block_id++; | ||
|
|
||
| $all_pages = get_pages(); |
There was a problem hiding this comment.
I noticed that my page with an order of 0 is appearing at the end (after items with an order greater than 0). It looks like we might need to use some of the params for get_pages (https://developer.wordpress.org/reference/functions/get_pages/).
There was a problem hiding this comment.
Looking closer, I was unsure if this would work because the resulting array that is built below ($top_level_pages) uses a key rather than an array index. In JavaScript that would result in the array being ordered by the key (or if using an object not ordered at all), but it looks like that's not the case in PHP:
https://stackoverflow.com/questions/10914730/are-php-associative-arrays-ordered
You can put them in whatever order you like, even when using numbers. Weird!
There was a problem hiding this comment.
Yeah these are really objects, not arrays as we understand them in JS 😅
I'm not sure what the page order setting is supposed to do, apart from changing the order of display in wp-admin/edit.php?post_type=page. Trying to add all pages to a classic menu, the numbered ones are sitting between 0s too 🤷♀️
I'd expect get_pages to offer a way of sorting them by order but it doesn't seem to, and I'm following the default order get_pages returns them in (apparently ASC).
There was a problem hiding this comment.
The pages widget does sort in the right order though, so probably good to replicate that, and it seems like an improvement over the old menus.
For get_pages, the sort_column argument should make sure the order is used for sorting. It's a shame there's no way to specify a secondary sort.
get_pages( array( sort_column => 'menu_order' ) );There was a problem hiding this comment.
I did some more investigation into this, as I became intrigued by the hierarchical argument of get_pages, which is always true. I couldn't figure out what this did until I looked at the code.
It looks like if you do get_pages( array( 'sort_column' => 'menu_order' ) ), it by default returns all the pages in an order that takes the nesting into account. The return value is a flat array, but the nested items are in the correct position. It looks like it should be possible to simplify how the nesting works in this block.
There was a problem hiding this comment.
I'm not sure there's much simplification can be done, as we still need to nest child pages under parents to generate the proper markup, so we need the pages to be indexed by their ID, which they're not when output by get_pages. I can't think of an easier way of doing this, but suggestions welcome!
There was a problem hiding this comment.
Yeah, it is still pretty tricky. I expected hierarchical to build a nested array much like you've done in this PR. I think to render in this format, the rendering loop would have to walk ahead to the next items to determine if they're children, and then add the nesting markup when that's the case.
Does feel a bit like an unnecessary refactoring given what's in this PR works, so maybe not worth it.
It could still be a decent optimisation to set hierarchical option to false in the pages block, as get_pages does a whole bunch of work that's not needed:
| /** | ||
| * Renders the `core/page-list` block on server. | ||
| * | ||
| * @param array $attributes The block attributes. | ||
| * @param array $content The saved content. | ||
| * @param array $block The parsed block. | ||
| * | ||
| * @return string Returns the page list markup. | ||
| */ |
There was a problem hiding this comment.
Seems like this became indented
talldan
added
[Type] Feature
|
|
||
| if ( $block->context && $block->context['showSubmenuIcon'] ) { | ||
| $css_classes .= ' show-submenu-icons'; | ||
| } |
There was a problem hiding this comment.
I would move this to it's own setter like block_core_page_add_context_classes( $context, $classes ).
There was a problem hiding this comment.
Would that include just this if statement, or the whole classname setting logic? The colours and font-size classes set above are also derived from context.
With the submenu icons in particular, I'm hoping we can remove this class soon, and just render the submenu icons whenever they are needed. The problem is we're server side rendering the block in the editor, and ServerSideRender doesn't recognise context. There's some more discussion on this problem in #28684 . Anyway, I'd leave this as is for now and revisit later pending the outcome of that discussion.
|
🔥 |
| @@ -148,6 +149,7 @@ export const __experimentalGetCoreBlocks = () => [ | |||
| missing, | |||
| more, | |||
| nextpage, | |||
| pageList, | |||
There was a problem hiding this comment.
So this block is stable and independent of the navigation block, which means it's going to be included in the next WP major (5.8) regardless of the status of the Navigation block, I just want to make sure this is a conscious decision.
There was a problem hiding this comment.
@tellthemachines was the block meant to be tweaked with regards to syntax, or is it pretty much baked as is?
There was a problem hiding this comment.
It seems we should keep this tied to the navigation block (and its child blocks) for a bit
There was a problem hiding this comment.
In making this an independent block, I was thinking of the Widgets editor, as we don't currently have another block that can reproduce the Pages widget functionality. Assuming we want the Widgets editor to have feature parity with the legacy widgets screen, this block will be needed.
I'm happy with its current syntax, and the fact that it gets whatever styling properties from Navigation block context means that when used standalone, it works just like the Categories or the Archives block, that don't have any styling options.
Related question: should Categories also work as a child of Navigation? There's definitely a use case for categories in menus.
| "customBackgroundColor", | ||
| "fontSize", | ||
| "customFontSize", | ||
| "showSubmenuIcon" |
There was a problem hiding this comment.
It feels like "context" is being abused in the navigation block, what happens if I use this block independently and I want to customize these things?
There was a problem hiding this comment.
Looked a bit at this. It relates to the block support mechanism and parent/child relationship.
Braindump of what we have now:
- a block wants to serialize styles to its wrapper => default supports mechanism
- a block wants to serialize styles to any other node => support mechanism with skip serialization (but only for its own markup, can't affect children)
- a block wants to serialize styles to children blocks or or a children wants to take styles from the parent => block context
One thing we could explore would be to allow blocks to automatically expose some block supports via context without having to declare context support explicitely. Was that what you had in mind, Miguel?
There was a problem hiding this comment.
I added the context in to match the Navigation Link block, so that all the links in the Navigation could be styled equally. When Page List is used as an independent block, those styling options aren't available. This seemed ok as the standalone block reproduces the functionality of the Pages widget, and similar blocks like Latest Posts also have limited styling.
As for the styling mechanism used I think we should go with the same strategy for both Navigation Link and Page List so that they work consistently inside the Navigation block, but I'm not super knowledgeable about how Global Styles work so not sure what it might look like!
There was a problem hiding this comment.
One thing we could explore would be to allow blocks to automatically expose some block supports via context without having to declare context support explicitely. Was that what you had in mind, Miguel?
Actually, I'm much less concerned about explicitly including Global Styles / Block Supports keys under usesContext, than I am about manually replicating some of gutenberg_apply_colors_support as part of render_block_core_page_list (see block_core_page_list_build_css_colors). This is where I think it'd be best to see where, if anywhere, GS/BS could improve to accommodate scenarios like Page List.
There was a problem hiding this comment.
In playing with this code I've noticed the block attributes and the block context have the same shape, so we can apply the same transformations on that data to create classes and styles ― it's a matter of finding how.
A small step in making both cases more similar could be #32807 (experimented only with colors to see how it feels).
| "customBackgroundColor", | ||
| "fontSize", | ||
| "customFontSize", | ||
| "showSubmenuIcon" |
| static $block_id = 0; | ||
| $block_id++; | ||
|
|
||
| $all_pages = get_pages( array( 'sort_column' => 'menu_order' ) ); |
There was a problem hiding this comment.
@tellthemachines: Is there a discussion somewhere in GH about limiting this query with number?
There was a problem hiding this comment.
I don't recall there being any discussion on that. I did test it with lots (several hundreds) of pages and it works ok because the block is server rendered. If we wanted to render it on the client side in the editor (I recall some talk about this at one point - possibly with @gwwar ?) it might be a different matter.
There was a problem hiding this comment.
I think some reasonable limit could be set for both server/client. Any client side implementation will hit issues much sooner (eg needing to fetch all results via API+ potentially adding a ton of navigation links).
There's should already be a limit for the page-list -> navigation links conversion, where we won't display the option if there are too many pages.
Description
Closes #23689 .
Adds a dynamic Pages block that updates with new pages and can be added inside a Navigation block or used on its own.
Update: this is now ready for review.
I am usingwp_list_pagesto output the list of pages, and have extendedWalker_Pageto add the chevron to items containing children. Apart from that, I have kept the default output classnames. If we'd rather be more consistent with other block classnames, I can further extend the walker to change theliand submenuulclasses, but can't do much more than that, e.g. theaelements aren't changeable. Cc @shaunandrews @jasmussenUpadate 2: This is now querying the pages and building the nested list manually; classnames are semantically closer to the Navigation block but not context-aware. See here for more details.
The editor view is using
ServerSideRenderbecause there's a notable performance benefit relative to rendering REST API data with JS. This means there's an extradivwrapper around the block contents in the editor, which there doesn't seem to be any way to stopServerSideRenderfrom outputting.When the block is nested inside a Nav block, all settings are inherited except for custom colors, because there's a problem with how inline styles are set for Nav block children that needs to be solved separately.
I haven't added any fixtures or other tests yet, will do that once we determine the markup is all correct 😅Tests added/fixed.Speaking of which, this block rendering a
ulcontainer is inconsistent with the current Nav block markup which expects all its children to belielements. I'm expecting that to be fixed by #24644 / #24364. The Pages block shouldn't be contained in alias it can be used separately from the Nav block.How has this been tested?
Screenshots
Types of changes
Checklist: