Page List: add ability to convert to navigation links #30390
Conversation
|
@jasmussen feel free to hop onto the branch directly if you see any styling updates to apply. I noticed the mock had a dialog without a title. I ended up using a modal (since this was the closest thing I spotted in storybook). Unfortunately I don't think I can turn off the title as an option in the component params / and I can't add a custom class to the modal instance.
|
|
Size Change: +4.68 kB (0%) Total Size: 1.42 MB
ℹ️ View Unchanged
|
gwwar
force-pushed
the
add/edit-page-list
branch
from
8c2e917 to
4dbea0c
Compare
|
This is amazing! I'm AFK for the rest of the week, but if this can wait until Monday, I'll push some polish to it, okay? Thank you! |
|
@jasmussen Sounds good! No rush on this one, I still have a few areas to polish on the PR. |
|
General Design Question for Page List (that don't need to necessarily be addressed in this PR): Should we add limits to how many pages we display in page list or navigation?Certain sites can have thousands of pages, especially if they're being used like a wiki. Here's a small example of me generating only 250 pages then starting a navigation menu: It already feels pretty silly. I just wanted to verify here since it's not a great user experience when there are lots of pages, and having unbounded values that we fetch/process make it very difficult to make this performant. Similarly I think pages can be nested to any depth, but we have work in progress to limit to 5.
|
|
🙈 Not related to changes here, but adding ~250 navigation-links on the editor slows it down tremendously. lag.mp4 |
|
Due to existing perf issues, I think we'll want to make edit unavailable if pages are > X. Where the limit is probably under 100 or so. Note that conversion isn't producing the correct results when working across page request boundaries eg > 100 pages. Edit: hmm I can't get REST API queries to match, I'll circle back to this, but I do wonder if this affects other areas like random ordering for assigning a parent page. eg |
|
Beautiful work! I pushed some polish:
I removed the "ButtonBroup" container, as currently it's used for a different style of buttons:
|
|
Thanks @jasmussen for the styling updates 💖 |
…t button until pages are available. Drop entities change.
| 'core/navigation-link', | ||
| { | ||
| id, | ||
| label: title.rendered, //TODO: raw or rendered? |
There was a problem hiding this comment.
Not sure what the difference is here between raw or rendered 🤔
There was a problem hiding this comment.
The only difference I've found is if a page has no title, raw will be an empty string while rendered will be "Untitled". The latter is probably preferable here as an empty nav link would be confusing. The Page list block itself uses the raw title because that's what's returned from get_pages.
|
Thanks for the review @jasmussen ! It looks like I have a little bit more work here, which I plan on addressing:
|
|
Added a basic description for screenreaders:
As an aside, the modal component could use some 🤔 for making this a bit easier out of the box. Labels are added pretty much automatically, but then the describedby label is more hidden with an API for passing in a single aria object. gutenberg/packages/components/src/modal/index.js Lines 133 to 137 in 6604dd3 |
|
So in the REST API we're powering the DB query using WP_Query. This generates a more complicated query than |
|
🤔 With a number of results that are less than the page limit (for example 11 published), we're seeing different ordering results when
query.mp4Any recommendations here on getting a stable sort order from the API + this block render_callback? Maybe @talldan @TimothyBJacobs |
|
For completeness, here's the query from the original get_pages call:
|
|
Oh fun, I'm seeing the sorting flip when we limit at 499 vs 500. I wonder if I'm hitting an internal implementation detail: 499.mp4 |
There was a problem hiding this comment.
I left a few minor comments/questions, but overall the code looks good!
Folks have also expressed a desire to remove the usage of but let's leave that for a follow up.
Fwiw, I implemented it that way initially because performance in the editor with >100 or so pages was pretty horrific 😬
| 'core/navigation-link', | ||
| { | ||
| id, | ||
| label: title.rendered, //TODO: raw or rendered? |
There was a problem hiding this comment.
The only difference I've found is if a page has no title, raw will be an empty string while rendered will be "Untitled". The latter is probably preferable here as an empty nav link would be confusing. The Page list block itself uses the raw title because that's what's returned from get_pages.
| </p> | ||
| <p> | ||
| { __( | ||
| "Note: if you add new pages to your site, you'll need to add them to your navigation menu." |
There was a problem hiding this comment.
This paragraph won't be read out as part of the description, is that ok?
There was a problem hiding this comment.
Can you clarify this? I'm seeing it in VoiceOver:
There was a problem hiding this comment.
Oh, it's read out if you're manually arrowing through the whole content of the modal, but not when you first land on it. So, if you press "Edit", the modal opens and "Convert to links" is read out, followed by the first paragraph of the content. If user just presses tab after that, they'll skip through the buttons and never reach the rest of the text.
I guess that might be ok though, as the note just relays complementary info and isn't essential to understand what the "convert" action does.
| > | ||
| <p id={ 'wp-block-page-list-modal__description' }> | ||
| { __( | ||
| 'To edit this navigation menu, convert it to single page links. This allows you to add re-order, remove items, or edit their labels.' |
There was a problem hiding this comment.
A comma between "add" and "re-order" would help the spoken description sound more clear.
| { | ||
| per_page: MAX_PAGE_COUNT, | ||
| _fields: PAGE_FIELDS, | ||
| orderby: 'menu_order', |
There was a problem hiding this comment.
Funny that this seems to return the pages in more or less oldest to most recent order, whereas the same parameter passed to get_pages returns a completely different order 😕
There was a problem hiding this comment.
This is because get_pages has a default order of asc and WP_Query (used by the REST API) has a default order of desc, I'll double check which one we need to respect for menu_order.
It looks like we can't expect a stable sort from using menu_order, when items are equal to each other (eg menu_order of 0), so I'll need another dimension to sort on in addition to menu_order. Do folks prefer title alphabetizing or something like post date?
There was a problem hiding this comment.
For what it's worth, I don't think it's crucial to get this 100% right in this one PR. It's can be a nice enhancement in the future, but the value being able to convert the page list outweighs the downside that sorting might not be perfect, especially paired with the fact that you get immediate access to mover controls right after the conversion.
There was a problem hiding this comment.
Looks like it won't be possible to provide a consistent ordering until we can implement https://core.trac.wordpress.org/ticket/39037.
Fwiw, I implemented it that way initially because performance in the editor with >100 or so pages was pretty horrific
Good to know @tellthemachines! I'll look into adding multiple orderby support in the REST API next, then start on some general performance profiling for Navigation Links.
I've incorporated feedback and added a few explanations in e35f132
…m render_callback vs rest api
|
Thanks for the reviews @jasmussen @tellthemachines ! |
Fixes #29437.
This PR allows the Page List block to be converted to single menu items, so those can be reordered, deleted or tweaked at will.
example.mp4
Implementation Notes of Interest
This is the straightforward approach specific to the page list. Folks have also expressed a desire to remove the usage of
<ServerSideRender />but let's leave that for a follow up.Problems reserved for follow up PRs:
get_pages( array( 'sort_column' => 'menu_order' ) )when usingorderby: menu_order. This is noticeable when results are > 100 (meaning we have multiple fetches). I suspect this might be a more general issue. Let me know if folks spot the right combination.Testing Instructions
On a site with < 100 pages:
On a site with > 100 pages:
Add a page list directly into the page
The Problem
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: