-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
[Query block]: Add Posts List variation #26990
Conversation
Size Change: +295 B (0%) Total Size: 1.19 MB
ℹ️ View Unchanged
|
ac6e63e
to
37e725c
Compare
Thanks, @ntsekouras! Can we make sure the Placeholder screen updates to reflect the block name? I noticed the Embed block does this. Example below. But this particular block variations isn't acting this way. |
I'll check it out @mapk - thanks for reviewing. I'll see what I can do to extend the Variation Picker component. |
pages: 1, | ||
offset: 0, | ||
postType: 'post', | ||
categoryIds: [], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's pretty unfortunate that it needs to be all duplicated here because of how the query object was shaped:
gutenberg/packages/block-library/src/query/block.json
Lines 12 to 23 in 9438924
"perPage": null, | |
"pages": 1, | |
"offset": 0, | |
"postType": "post", | |
"categoryIds": [], | |
"tagIds": [], | |
"order": "desc", | |
"orderBy": "date", | |
"author": "", | |
"search": "", | |
"exclude": [], | |
"sticky": "" |
It raises a question of whether all those settings shouldn't be top-level attributes. It's clearly out of scope for this PR but I wanted to mention it. I understand that it's simpler to pass query
through context this way, but at the same time, we lose some benefits that more granular attributes provide, like type validation, the flexibility to change the default value without breaking backward compatibility, less metadata saved in the content when defaults are used. An interesting fact is that the Latest Posts block defines all attributes as top-level fields:
gutenberg/packages/block-library/src/latest-posts/block.json
Lines 5 to 83 in d3fd39a
"attributes": { | |
"categories": { | |
"type": "array", | |
"items": { | |
"type": "object" | |
} | |
}, | |
"selectedAuthor": { | |
"type": "number" | |
}, | |
"postsToShow": { | |
"type": "number", | |
"default": 5 | |
}, | |
"displayPostContent": { | |
"type": "boolean", | |
"default": false | |
}, | |
"displayPostContentRadio": { | |
"type": "string", | |
"default": "excerpt" | |
}, | |
"excerptLength": { | |
"type": "number", | |
"default": 55 | |
}, | |
"displayAuthor": { | |
"type": "boolean", | |
"default": false | |
}, | |
"displayPostDate": { | |
"type": "boolean", | |
"default": false | |
}, | |
"postLayout": { | |
"type": "string", | |
"default": "list" | |
}, | |
"columns": { | |
"type": "number", | |
"default": 3 | |
}, | |
"order": { | |
"type": "string", | |
"default": "desc" | |
}, | |
"orderBy": { | |
"type": "string", | |
"default": "date" | |
}, | |
"displayFeaturedImage": { | |
"type": "boolean", | |
"default": false | |
}, | |
"featuredImageAlign": { | |
"type": "string", | |
"enum": [ | |
"left", | |
"center", | |
"right" | |
] | |
}, | |
"featuredImageSizeSlug": { | |
"type": "string", | |
"default": "thumbnail" | |
}, | |
"featuredImageSizeWidth": { | |
"type": "number", | |
"default": null | |
}, | |
"featuredImageSizeHeight": { | |
"type": "number", | |
"default": null | |
}, | |
"addLinkToFeaturedImage": { | |
"type": "boolean", | |
"default": false | |
} | |
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I know but this applies for all blocks with object
properties. We had discussed that in my PR for this:#26162, but for back compat it was closed.
It raises a question of whether all those settings shouldn't be top-level attributes.
This is something to consider for sure. With my work on Query, I don't think it will have many other properties, so a rework could be considered.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
Hey @mapk - I checked again the code for With --edit: Actually I exposed and used the simple implementation for matching a block variation and now is showing the variation's title in the Placeholder. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's move forward with the current implementation if @mapk is fine with how it works at the moment.
I think that using __experimentalGetMatchingVariation
is fine for now. Assuming it turns out to be useful as a general-purpose utility, we might need to convert it to a store selector at some point.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, @ntsekouras!
I think that icon for the block is getting over used, but we can revisit that in another PR.
Description
Resolves: #26193
This PR adds a simple Query block variation of Posts List, which shows latests
posts
excluding thesticky
posts. After selecting this variation from theinserter
, the Query's Placeholder is shown to select a 'design'(InnerBlocks actually) variation, like Post Title + Post Excerpt.In this PR I have also exposed the
__experimentalGetMatchingVariation
util to have better information in the Placeholder of Query block.