Remove unstable max pages attribute from Nav block#36877
Merged
Conversation
38 tasks
talldan
approved these changes
Nov 26, 2021
Contributor
talldan
left a comment
There was a problem hiding this comment.
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'] ); |
Contributor
There was a problem hiding this comment.
I wonder if it should be an argument to get_pages, usually that supports adding a limit.
Contributor
Author
There was a problem hiding this comment.
Good point. Let's consider a followup.
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
Contributor
|
There's now a feature request (#37340) for finishing this feature off in the page list block. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
In #36724 we added a
__unstableMaxPagesattribute 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?
wp_navigationPosts from your site.Save.4items displayed.Screenshots
Types of changes
Checklist:
*.native.jsfiles for terms that need renaming or removal).