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

Remove unstable max pages attribute from Nav block #36877

Merged
merged 2 commits into from
Nov 26, 2021

Conversation

getdave
Copy link
Contributor

@getdave getdave commented Nov 25, 2021

Description

In #36724 we added a __unstableMaxPages attribute to both Page List and Navigation blocks. When used inside the Nav block the Page List would receive the property value via context from the Nav block.

This seems like overengineering things...and it is(!), but only because we assumed that a lack of existing attributes on the Page List block was an intentional architectural decision so we decided using context was more appropriate and followed prior art in the code.

In hindsight all we achieved was propagating a potentially unstable attribute onto two blocks instead of one. Moreover @talldan explained to us that the Page List block is entirely open for extension using attributes and does not need to rely on context.

This PR reverses the original decision and removes the attribute from the Nav block. It retains it on the Page List block and updates the Nav block front end rendering to set the attribute directly on the Page List block itself rather than via context on the Nav block.

How has this been tested?

  • Add Pages to your site.
  • Delete all Navigation Menus wp_navigation Posts from your site.
  • Add an empty Nav block into the Site Editor and hit Save.
  • Go to frontend of site.
  • Check that the Page List block renders with 4 items displayed.

Screenshots

Types of changes

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • I've tested my changes with keyboard and screen readers.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR (please manually search all *.native.js files for terms that need renaming or removal).

@getdave getdave self-assigned this Nov 25, 2021
@getdave getdave added [Block] Navigation Affects the Navigation Block [Block] Page List Affects the Page List Block labels Nov 25, 2021
@getdave getdave marked this pull request as ready for review November 25, 2021 16:13
@getdave getdave added 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 Nov 25, 2021
@getdave getdave mentioned this pull request Nov 25, 2021
38 tasks
Copy link
Contributor

@talldan talldan left a comment

Choose a reason for hiding this comment

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

Good step in the right direction.

I notice that it works in the frontend only, we should take some to follow-up and also make it work in the editor after the beta is cut.

$nested_pages = array_slice( $nested_pages, 0, $block->context['__unstableMaxPages'] );
// Limit the number of items to be visually displayed.
if ( ! empty( $attributes['__unstableMaxPages'] ) ) {
$nested_pages = array_slice( $nested_pages, 0, $attributes['__unstableMaxPages'] );
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if it should be an argument to get_pages, usually that supports adding a limit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. Let's consider a followup.

@getdave getdave merged commit c9e32e0 into trunk Nov 26, 2021
@getdave getdave deleted the try/remove-unstable-max-pages-from-nav-block branch November 26, 2021 09:53
@github-actions github-actions bot added this to the Gutenberg 12.1 milestone Nov 26, 2021
@noisysocks noisysocks 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 Nov 29, 2021
noisysocks pushed a commit that referenced this pull request Nov 29, 2021
* Move to attr on Page List and remove from Nav block

* Use empty check incase attr is falsey but present
@talldan
Copy link
Contributor

talldan commented Dec 14, 2021

There's now a feature request (#37340) for finishing this feature off in the page list block.

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants