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

Removes __unstableMaxPages attribute from Page List block (and Nav block) #44415

Merged
merged 2 commits into from
Oct 5, 2022

Conversation

getdave
Copy link
Contributor

@getdave getdave commented Sep 23, 2022

What?

Removes the unstable attribute which was being tested to allow limiting of number of Page List pages being shown.

This PR also removes the attribute from the Nav block and will cause the Navigation block fallback page list to display unlimited numbers of pages (cc @jameskoster @jasmussen).

Closes #44360

Why?

See discussion here #44360.

Essentially most contributors felt this prop is unnecessary and should be omitted.

How?

Remove attribute and any related implementation.

Testing Instructions

  • Add 10 or so Pages
  • New Post
  • Switch to code view and add <!-- wp:page-list {"__unstableMaxPages":4} /-->
  • You should see all your 10 Pages. Should not be limited.
  • Switch to front of site and confirm the same thing happens.

Screenshots or screencast

@getdave getdave added the [Block] Navigation Affects the Navigation Block label Sep 23, 2022
Copy link
Contributor

@scruffian scruffian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this. I tested and it behaves as expected. I also wondered if we needed a block deprecation, but in my testing that didn't seem to be a problem. I also tested that this works as expected with the fallbacks in the navigation block.

Do we need to be a bit careful? What if someone had a million pages? Could this make their site unusable? Would that already be the case?

@getdave getdave added [Type] Bug An existing feature does not function as intended Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta labels Sep 23, 2022
Copy link
Contributor

@carolinan carolinan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This works well in my test.
Let's consider Ben's question above.

@getdave
Copy link
Contributor Author

getdave commented Sep 26, 2022

Do we need to be a bit careful? What if someone had a million pages? Could this make their site unusable? Would that already be the case?

Noting that there's a hard limit of 100 in the block.

@jasmussen
Copy link
Contributor

Hey, thanks for this PR! Just catching up on the context, I would agree that the default hard lock of 100 captures edge-case sites. But also with the main purpose of this PR, to have just that, and no additional limits. The page list block is mainly created for that purpose, to just list all of your blocks. When used in the navigation block and especially with the recent fallback behaviors, can provide an default behavior for most users.

I would argue that if you have a lot of pages, too many for the menu, you're likely better served by pressing "Edit" to convert the menu to a fully customizable one. This is even despite the fact that custom menu editing still isn't the most intuitive, which is to say we have work there. I like to think that #42257 (comment) can play a key factor there.

Copy link
Contributor

@draganescu draganescu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this the change we need? Should we try to harmonize the editor and the front end and allow people to remove/edit this limit instead? I was looking to find reasoning behind this attribute, I found:

  1. The attribute was introduced in Implement suitable fallback for Nav block on front end of site when no menu selected #36724 as an attribute of the navigation block
  • I could not find in commit message, PR comments or description:
    • why did we introduce it?
    • why was it unstable?
    • why "4" was the number?
  • As far as I can remember, at the time there was a huge amount of effort and change related to the navigation block, all at once, by a very small number of contributors, so it is likely the discussion is somewhere but not referenced (or I missed it).
  • It would be useful to remember the reasoning back then.
  1. The attribute was moved from the navigation block to the page list block in Remove unstable max pages attribute from Nav block #36877
  • the original intention for a follow up there was to harmonize the behavior in the editor to match the behavior of the page list render function.
  • this didn't occur for quite some time and now we're removing it from everywhere.
  • so for a second PR it seemed this was a good thing, we just refactored the code.
  1. The answer to @scruffian 's question

Do we need to be a bit careful? What if someone had a million pages?

@getdave
Copy link
Contributor Author

getdave commented Sep 27, 2022

  • why did we introduce it?

As memory serves it was introduced after it was transferred from the Nav block. The reason was it was there to avoid a situation where a site with a lot of top level pages sees a massive Navigation when the Page List fallback gets created.

  • why was it unstable?

It was a quick fix for the front of site only. There was no editor implementation. It was never intended as a user-facing change. Moreover we weren't 100% that it was necessary - thus the "unstable". I think given the comments by @jasmussen (above) and others that was the right call.

  • why "4" was the number?

Arbitrary decision. Open to change.

We already track this issue #37340 from direct user feedback where a site with "90+ pages" find defaulting to "show all" problematic.

This is a good find @draganescu. So we do have feedback from users saying they want a limit.

The debate here is that some contributors feel limiting the number of pages is not something we should be doing on the Page List. Others feel that having a "hidden" attribute that can be used in certain specific situations to limit Pages isn't harmful and should be retained.

I raised this PR to start a debate about whether or not to retain the prop because it's been sitting as "unstable" for some time now.

My personal opinion is that having a hidden attribute and making use of that in the Nav block is reasonable. I appreciate however that others disagree.

How shall we move forward?

@jameskoster
Copy link
Contributor

I think it's important not to blur the lines between the Navigation block and Page List. If we have an option to limit the number of pages, next comes a request to re-order the pages via the block. Then to not include dropdowns. Then to add custom links. Then to save different versions... Before you know we've recreated the Navigation block :)

As for a hidden limit to cover performance edge cases... is there a precedent for this? Does the Query block have such an affordance?

@draganescu
Copy link
Contributor

Does the Query block have such an affordance?

Yes. This is built in by pagination's per page limit.

Anecdotally I once tested a block theme which had a query block with no pagination with the post limit set to 999 and my blog froze the browser on the homepage upon visit. The editor worked but I am on a highly performant machine.

I think it's important not to blur the lines between the Navigation block and Page List.

Yes, limiting the number of menu items for design reasons should be a feature of the navigation block. But since this falls back to a page list how can we tell it to respect the theme's choice? Page List displays a tree of items all we care about is how many top level items do we allow, and maybe set a sensible default.

Historically, I think get_pages wp_list_pages and wp_page_menu none has any default limit on total number of pages nor an easy way to enforce one, so a theme wanting to surface that had to do a lot of work I believe.

@talldan
Copy link
Contributor

talldan commented Sep 30, 2022

I think there was an issue out there that suggested there could be a limit to the top level pages, and then any after that are shown in an overflow 'more' menu. That feels like a better option.

The 'more' menu could still get quite huge though. Perhaps the 'more' menu could have a 'more' menu too. :trollface:

I would still consider that more of a 6.2 thing. It's probably worth looking at a fix for 6.1 to make the editor and frontend the same. I don't really have a strong opinion on whether it's four items or a hundred. I would maybe lean towards four, because a hundred is likely to make the template look really awful, and I don't think it's all that easy to edit things as it stands in 6.1. There's still that issue about Page Lists being hard to select.

@jasmussen
Copy link
Contributor

I like to think of the max value as mainly a bounds to catch edgecases. But any low number feels wrong since the page list was created to show all pages. Its presence in the navigation block is to provide a good default, which it arguably will in 80% of sites that have only a slew of pages in total. For all those cases with many more blocks, it's highly likely they'd want an entirely custom menu anyway, and any low bounds we set in a page list would probably not work regardless (and be at the mercy of page sort order as well). Editing the menu to delete pages, or just creating an entirely new menu, feels like the interface to optimize for this use case.

@draganescu draganescu dismissed their stale review October 3, 2022 13:22

Feedback received

@draganescu
Copy link
Contributor

How shall we move forward?

I think let's merge this and drop the attribute, as it seems rather:

  • artificial, it does not serve a purpose for the block itself but rather a guard for some edge case
  • random in its value
  • put in the wrong block (I agree the pages list block should show all pages - or if we want to limit this it should be all the way with an UI for it etc.)

@jasmussen what can we do about the horrible experience of having your layout broken simply because you have many pages?

  • WP does not auto migrate classic menus yet.
  • A user with 90 pages and a classic menu only testing a block theme will see a horrible thing.

Should we just wait it out until figure out #44173 ? I think that once that one lands, having (a) tens/hundreds of pages and (b) no menus is definitely an edge case.

@jasmussen
Copy link
Contributor

How widespread of a problem is this? #44360 appears to show test content, and I don't know that's sufficient to make a decision on. And on the flipside, I think Carolina makes a good argument for why arbitrarily limiting the page list comes with its own set of problems. I understand that there is already a built-in limit of 100 pages in the block, which IMO seems fine to keep as is until we can actually confirm this is a problem. Especially when paired with the ability to edit a menu, to either start fresh with an empty menu where you manually curate, or edit the existing page list to delete the ones you don't want.

@getdave getdave changed the title Removes __unstableMaxPages attribute from Page List block Removes __unstableMaxPages attribute from Page List block (and Nav block) Oct 4, 2022
@getdave
Copy link
Contributor Author

getdave commented Oct 4, 2022

OK so let's merge this PR. Agreed?

👍 / 👎

@jasmussen
Copy link
Contributor

This PR also removes the attribute from the Nav block and will cause the Navigation block fallback page list to display unlimited numbers of pages

It seems fine to have a boundary, just like the Pages list in wp-admin has one. But it should just be high, and hard-coded, not something we want a UI or an API for, IMO.

@mtias
Copy link
Member

mtias commented Oct 5, 2022

We should not artificially limit page list count to cover a deficiency in our migration.

A user with 90 pages and a classic menu only testing a block theme will see a horrible thing.

We need to ensure a classic menu, if present, is privileged. That's the issue to solve.

@getdave getdave merged commit 728832a into trunk Oct 5, 2022
@getdave getdave deleted the remove/unstable-max-limit-from-page-list branch October 5, 2022 14:21
@github-actions github-actions bot added this to the Gutenberg 14.4 milestone Oct 5, 2022
@getdave
Copy link
Contributor Author

getdave commented Oct 5, 2022

I went ahead and merged this as there appears to be a general consensus that we shouldn't be artificially limiting the Page List.

If/when the lack of limit (other than the hard limit of 100) becomes a problem we can revisit this.

ockham pushed a commit that referenced this pull request Oct 10, 2022
…ock) (#44415)

* Removes unstable limit attribute

* Remove attribute from Nav block
@ockham
Copy link
Contributor

ockham commented Oct 10, 2022

I just cherry-picked this PR to the wp/6.1 branch to get it included in the next release: c8bc6f5

@ockham ockham removed the Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta label Oct 10, 2022
pento pushed a commit to WordPress/wordpress-develop that referenced this pull request Oct 11, 2022
Package updates for bug and regression fixes:
@wordpress/block-directory: 3.15.6
@wordpress/block-editor: 10.0.6
@wordpress/block-library: 7.14.6
@wordpress/components: 21.0.5
@wordpress/customize-widgets: 3.14.6
@wordpress/edit-post: 6.14.6
@wordpress/edit-site: 4.14.8
@wordpress/edit-widgets: 4.14.6
@wordpress/editor: 12.16.6
@wordpress/format-library: 3.15.6
@wordpress/interface: 4.16.5
@wordpress/list-reusable-blocks: 3.15.5
@wordpress/nux: 5.15.5
@wordpress/preferences: 2.9.5
@wordpress/reusable-blocks: 3.15.6
@wordpress/server-side-render: 3.15.5
@wordpress/widgets: 2.15.6

References:
* [WordPress/gutenberg#43181 Gutenberg PR 43181] - Merge inner blocks if wrappers are equal
* [WordPress/gutenberg#44098 Gutenberg PR 44098] - Fix content blocks with nested blocks always appear as top level
* [WordPress/gutenberg#44150 Gutenberg PR 44150] - Refresh selection styles
* [WordPress/gutenberg#44415 Gutenberg PR 44415] - Removes `__unstableMaxPages` attribute from Page List and Nav blocks
* [WordPress/gutenberg#44463 Gutenberg PR 44463] - Follow discussion settings in the comments block edit
* [WordPress/gutenberg#44584 Gutenberg PR 44584] - Avoid querying block templates during installation
* [WordPress/gutenberg#44637 Gutenberg PR 44637] - Resizable editor: Fix height setting bug
* [WordPress/gutenberg#44640 Gutenberg PR 44640] - Only include theme.css if the theme declares support for wp-block-styles
* [WordPress/gutenberg#44650 Gutenberg PR 44650] - Fix empty links being created for the author's name comment
* [WordPress/gutenberg#44652 Gutenberg PR 44652] - Block Editor: Fix block search for non-Latin characters
* [WordPress/gutenberg#44660 Gutenberg PR 44660] - Legacy Group inner block wrapper should work with constrained layout
* [WordPress/gutenberg#44676 Gutenberg PR 44676] - Hide list items from content area of content locked blocks
* [WordPress/gutenberg#44718 Gutenberg PR 44718] - Margin visualizer: Apply negative value to margins with `calc()`
* [WordPress/gutenberg#44721 Gutenberg PR 44721] - Remove anchor support from the navigation block
* [WordPress/gutenberg#44731 Gutenberg PR 44731] - Buttons: Add specificity for the editor

Follow-up to [54257], [54335], and [54383].

Props bernhard-reiter, czapla, annezazu, cbravobernal, ndiego, bjorsch, nendeb55.
See #56467.

git-svn-id: https://develop.svn.wordpress.org/trunk@54483 602fd350-edb4-49c9-b593-d223f7449a82
markjaquith pushed a commit to markjaquith/WordPress that referenced this pull request Oct 11, 2022
Package updates for bug and regression fixes:
@wordpress/block-directory: 3.15.6
@wordpress/block-editor: 10.0.6
@wordpress/block-library: 7.14.6
@wordpress/components: 21.0.5
@wordpress/customize-widgets: 3.14.6
@wordpress/edit-post: 6.14.6
@wordpress/edit-site: 4.14.8
@wordpress/edit-widgets: 4.14.6
@wordpress/editor: 12.16.6
@wordpress/format-library: 3.15.6
@wordpress/interface: 4.16.5
@wordpress/list-reusable-blocks: 3.15.5
@wordpress/nux: 5.15.5
@wordpress/preferences: 2.9.5
@wordpress/reusable-blocks: 3.15.6
@wordpress/server-side-render: 3.15.5
@wordpress/widgets: 2.15.6

References:
* [WordPress/gutenberg#43181 Gutenberg PR 43181] - Merge inner blocks if wrappers are equal
* [WordPress/gutenberg#44098 Gutenberg PR 44098] - Fix content blocks with nested blocks always appear as top level
* [WordPress/gutenberg#44150 Gutenberg PR 44150] - Refresh selection styles
* [WordPress/gutenberg#44415 Gutenberg PR 44415] - Removes `__unstableMaxPages` attribute from Page List and Nav blocks
* [WordPress/gutenberg#44463 Gutenberg PR 44463] - Follow discussion settings in the comments block edit
* [WordPress/gutenberg#44584 Gutenberg PR 44584] - Avoid querying block templates during installation
* [WordPress/gutenberg#44637 Gutenberg PR 44637] - Resizable editor: Fix height setting bug
* [WordPress/gutenberg#44640 Gutenberg PR 44640] - Only include theme.css if the theme declares support for wp-block-styles
* [WordPress/gutenberg#44650 Gutenberg PR 44650] - Fix empty links being created for the author's name comment
* [WordPress/gutenberg#44652 Gutenberg PR 44652] - Block Editor: Fix block search for non-Latin characters
* [WordPress/gutenberg#44660 Gutenberg PR 44660] - Legacy Group inner block wrapper should work with constrained layout
* [WordPress/gutenberg#44676 Gutenberg PR 44676] - Hide list items from content area of content locked blocks
* [WordPress/gutenberg#44718 Gutenberg PR 44718] - Margin visualizer: Apply negative value to margins with `calc()`
* [WordPress/gutenberg#44721 Gutenberg PR 44721] - Remove anchor support from the navigation block
* [WordPress/gutenberg#44731 Gutenberg PR 44731] - Buttons: Add specificity for the editor

Follow-up to [54257], [54335], and [54383].

Props bernhard-reiter, czapla, annezazu, cbravobernal, ndiego, bjorsch, nendeb55.
See #56467.
Built from https://develop.svn.wordpress.org/trunk@54483


git-svn-id: http://core.svn.wordpress.org/trunk@54042 1a063a9b-81f0-0310-95a4-ce76da25c4cd
github-actions bot pushed a commit to platformsh/wordpress-performance that referenced this pull request Oct 11, 2022
Package updates for bug and regression fixes:
@wordpress/block-directory: 3.15.6
@wordpress/block-editor: 10.0.6
@wordpress/block-library: 7.14.6
@wordpress/components: 21.0.5
@wordpress/customize-widgets: 3.14.6
@wordpress/edit-post: 6.14.6
@wordpress/edit-site: 4.14.8
@wordpress/edit-widgets: 4.14.6
@wordpress/editor: 12.16.6
@wordpress/format-library: 3.15.6
@wordpress/interface: 4.16.5
@wordpress/list-reusable-blocks: 3.15.5
@wordpress/nux: 5.15.5
@wordpress/preferences: 2.9.5
@wordpress/reusable-blocks: 3.15.6
@wordpress/server-side-render: 3.15.5
@wordpress/widgets: 2.15.6

References:
* [WordPress/gutenberg#43181 Gutenberg PR 43181] - Merge inner blocks if wrappers are equal
* [WordPress/gutenberg#44098 Gutenberg PR 44098] - Fix content blocks with nested blocks always appear as top level
* [WordPress/gutenberg#44150 Gutenberg PR 44150] - Refresh selection styles
* [WordPress/gutenberg#44415 Gutenberg PR 44415] - Removes `__unstableMaxPages` attribute from Page List and Nav blocks
* [WordPress/gutenberg#44463 Gutenberg PR 44463] - Follow discussion settings in the comments block edit
* [WordPress/gutenberg#44584 Gutenberg PR 44584] - Avoid querying block templates during installation
* [WordPress/gutenberg#44637 Gutenberg PR 44637] - Resizable editor: Fix height setting bug
* [WordPress/gutenberg#44640 Gutenberg PR 44640] - Only include theme.css if the theme declares support for wp-block-styles
* [WordPress/gutenberg#44650 Gutenberg PR 44650] - Fix empty links being created for the author's name comment
* [WordPress/gutenberg#44652 Gutenberg PR 44652] - Block Editor: Fix block search for non-Latin characters
* [WordPress/gutenberg#44660 Gutenberg PR 44660] - Legacy Group inner block wrapper should work with constrained layout
* [WordPress/gutenberg#44676 Gutenberg PR 44676] - Hide list items from content area of content locked blocks
* [WordPress/gutenberg#44718 Gutenberg PR 44718] - Margin visualizer: Apply negative value to margins with `calc()`
* [WordPress/gutenberg#44721 Gutenberg PR 44721] - Remove anchor support from the navigation block
* [WordPress/gutenberg#44731 Gutenberg PR 44731] - Buttons: Add specificity for the editor

Follow-up to [54257], [54335], and [54383].

Props bernhard-reiter, czapla, annezazu, cbravobernal, ndiego, bjorsch, nendeb55.
See #56467.
Built from https://develop.svn.wordpress.org/trunk@54483


git-svn-id: https://core.svn.wordpress.org/trunk@54042 1a063a9b-81f0-0310-95a4-ce76da25c4cd
ootwch pushed a commit to ootwch/wordpress-develop that referenced this pull request Nov 4, 2022
Package updates for bug and regression fixes:
@wordpress/block-directory: 3.15.6
@wordpress/block-editor: 10.0.6
@wordpress/block-library: 7.14.6
@wordpress/components: 21.0.5
@wordpress/customize-widgets: 3.14.6
@wordpress/edit-post: 6.14.6
@wordpress/edit-site: 4.14.8
@wordpress/edit-widgets: 4.14.6
@wordpress/editor: 12.16.6
@wordpress/format-library: 3.15.6
@wordpress/interface: 4.16.5
@wordpress/list-reusable-blocks: 3.15.5
@wordpress/nux: 5.15.5
@wordpress/preferences: 2.9.5
@wordpress/reusable-blocks: 3.15.6
@wordpress/server-side-render: 3.15.5
@wordpress/widgets: 2.15.6

References:
* [WordPress/gutenberg#43181 Gutenberg PR 43181] - Merge inner blocks if wrappers are equal
* [WordPress/gutenberg#44098 Gutenberg PR 44098] - Fix content blocks with nested blocks always appear as top level
* [WordPress/gutenberg#44150 Gutenberg PR 44150] - Refresh selection styles
* [WordPress/gutenberg#44415 Gutenberg PR 44415] - Removes `__unstableMaxPages` attribute from Page List and Nav blocks
* [WordPress/gutenberg#44463 Gutenberg PR 44463] - Follow discussion settings in the comments block edit
* [WordPress/gutenberg#44584 Gutenberg PR 44584] - Avoid querying block templates during installation
* [WordPress/gutenberg#44637 Gutenberg PR 44637] - Resizable editor: Fix height setting bug
* [WordPress/gutenberg#44640 Gutenberg PR 44640] - Only include theme.css if the theme declares support for wp-block-styles
* [WordPress/gutenberg#44650 Gutenberg PR 44650] - Fix empty links being created for the author's name comment
* [WordPress/gutenberg#44652 Gutenberg PR 44652] - Block Editor: Fix block search for non-Latin characters
* [WordPress/gutenberg#44660 Gutenberg PR 44660] - Legacy Group inner block wrapper should work with constrained layout
* [WordPress/gutenberg#44676 Gutenberg PR 44676] - Hide list items from content area of content locked blocks
* [WordPress/gutenberg#44718 Gutenberg PR 44718] - Margin visualizer: Apply negative value to margins with `calc()`
* [WordPress/gutenberg#44721 Gutenberg PR 44721] - Remove anchor support from the navigation block
* [WordPress/gutenberg#44731 Gutenberg PR 44731] - Buttons: Add specificity for the editor

Follow-up to [54257], [54335], and [54383].

Props bernhard-reiter, czapla, annezazu, cbravobernal, ndiego, bjorsch, nendeb55.
See #56467.

git-svn-id: https://develop.svn.wordpress.org/trunk@54483 602fd350-edb4-49c9-b593-d223f7449a82
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 [Type] Bug An existing feature does not function as intended
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

Nav block Page List fallback displays too many Pages
9 participants