-
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 grid view #27067
Query block grid view #27067
Conversation
@@ -22,11 +22,19 @@ | |||
"exclude": [], | |||
"sticky": "" | |||
} | |||
}, | |||
"layout": { |
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.
This is the attempt for having a structure similar to __experimentalLayout
. Check PR's description for more info.
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.
Should it be experimental then?
Latest Posts block defines two attributes:
gutenberg/packages/block-library/src/latest-posts/block.json
Lines 39 to 46 in 3aeb89e
"postLayout": { | |
"type": "string", | |
"default": "list" | |
}, | |
"columns": { | |
"type": "number", | |
"default": 3 | |
}, |
We can always tweak providesContext
to support some grouping if that makes it easier:
"providesContext": {
"queryId": "queryId",
"query": "query",
"layout": {
"columns": "columns",
"type": "layout"
}
}
Size Change: +2.37 kB (0%) Total Size: 1.2 MB
ℹ️ View Unchanged
|
I absolutely love using this control to adjust the columns in this block! Really good work! However, I notice that the Columns control is in the sidebar under a header that says, "Display." This is accurate, but now the display settings are being split between the sidebar and the block's toolbar. I was relying on a logical separation of "Display settings" in the block's toolbar and other settings and filters in the sidebar. But I also don't think the Column RangeControl would work well in the block's toolbar dropdown, so I'm rethinking where to surface these various settings. Rethinking how these are grouped shouldn't hold up this PR. It worked well 👍 from a design perspective. |
Cool work, thank you for including a GIF! 🏅 I like the feature too. But I wonder if maybe it needs a simpler approach, for two reasons:
As I go on to elaborate, please let me first state that this is a really impressive PR! Beyond question. My feedback comes from zooming out a bit. And to expand on 1, here's an inspector screenshot: Right now those columns are fluid, and the grid gap is arbitrarily chosen. There's no trivial mechanism for a theme to customize this grid system, and no built in responsiveness. The end result is that this grid becomes very prescriptive of the end layout, in a way that I think goes against what #16998 tries to accomplish. To get this properly configurable for a theme, it would need a great deal of code to handle what would probably need to be global styles integration for customizability, and lots of smarts to be responsive. This would add complexity just for this block. To expand on 2, well, it doesn't exist yet, but whether it be CSS grid or flex or something else, I would imagine that be the exact provider of a grid for a block like this. Instead of adding software on a per-block basis, this "Site Block" or what we might call it, would be the grid provider, and any block inside would inherit from it. In absence of that, and until such a thing could land, a simpler approach might be to just do what the Latest Posts block does: It's a boolean choice, grid or no grid. While it provides some basic structural CSS, it's really trivial and easy for a theme to customize. Those two — boolean and simple — means that if/when a grid provider like the site block lands, it would be easy to attach posts inside to the grid it provides. It also comes with some built in responsiveness. What do you think? |
Hey @mapk
This is something we can easily move around so we just have to reach a decision about this, at least good enough for first iteration. It could be something like @jasmussen proposes as well to follow the Hey @jasmussen
Is using I think the minimum requirement would be this:
As I'm not the expert for the css approach, I'm open to suggestions.
This is the goal for later as I have discussed this before with Riad
Thanks both of you for taking a look here 👍 |
I guess I'm suggesting that we should be mindful of using explicit column sliders on a per-block basis, as it opens up a can of worms on responsiveness, and that can is perhaps opened when defined in a single place used by multiple blocks at once. In the mean time, the latest posts block has a column slider and CSS to match it. If we re-use that 1:1 at least it will be consistent? Also, I'll always defer to Riad. He's proven his wisdom enough times 😅 |
Starting small is always appealing to me :) So I also have a preference for flex + boolean (or string if we imagine more layouts than just these two later) attribute + being opinionated in the styles (set a decent with per column) + responsiveness. Now Nik proposes to use the "columns" attribute as a way to switch layouts (one column means we're using the default layout, 2 or more grid), while that works, I think having an explicit attribute for the layout is good especially if we imagine introducing more of these with different configs. Maybe "columns" doesn't make sense in all of them. -- I assume switching the layout here means adding a classname to the container, something like
|
+1. I think it makes sense to mirror what we already have in the latest posts block. We know that works well. To @jasmussen's notes above, CSS grid works best when it's dealing with a known number of items. It doesn't scale super well with a dynamic number of elements inside of it. So |
I think a combination of all these suggestions aligns best with the I'll revert back to using flex and show the icon in the Query's toolbar. |
049ef22
to
e6be09a
Compare
The changes to match the Latest Posts block work well for me. 👍 |
Co-authored-by: Ari Stathopoulos <aristath@gmail.com>
e6be09a
to
940e97e
Compare
I think this is ready for review now :) |
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.
Code looks good.
Tested and it works.
My existing theme breaks with this change but that is to be expected, that is the nature of an experimental theme. We could add a deprecation function but I don't think it's necessary.
Everything works beautifully and though I would like this to use CSS-grid, flexbox also works.
Note that with css-grid we'd be able to have responsive without the breakpoints, so the resulting CSS would be significantly smaller:
ul {
grid-template-columns: repeat(auto-fill, minmax(min(13em, 100%), 1fr));
grid-gap: 1.25em;
}
li {
width: 100%;
}
That being said, the current solution works fine and has greater compatibility with IE11 so IMO this is good to merge 👍
This is great! Thank you Nik @ntsekouras and everyone else! I look forward to having a kind of grid layout of images that have the same height and width. As it would create a nice symmetry of images. An example. https://demo.gretathemes.com/estar/blog/ Perhaps also a card view = border and show drop shadow around each. Sounds like I am mentioning additional sidebar options.... Thanks! |
Description
This PR will add support for Query block to show lists as a list or a grid. It's part of Query block missing functionality: #24934
Notes
I have added a wrapper
ul
in the markup ofQueryLoop
and each entity's blocks in ali
to be semantically correct. That seems fine to me, because it makes the styling possible and now all blocks are without any wrapper besides the main one forpost-content
. (cc @mtias )I followed the design suggestions in comments to match the
Latest Posts
controls for displaying list/grid.Since it's not clear yet what we'll do with
QueryLoop
regarding 'merging' withQuery
, I have added the grid settings and handling inQuery
even though they apply toQueryLoop
. For the record I don't think for now these blocks will be merged andQueryLoop
will probably remain as a wrapper, but that's a separate matter...In this implementation a single property is needed
columns
and can be declared as a single top level property, but I have declared it as an object in an attempt to maybe be more flexible to be extracted later into an implementation of @youknowriad 's__experimentalLayout
(check PR here: #26380). This can change..How to test this
Columns
attributeScreenshots
Other thoughts
If this PR goes well and lands eventually, by augmenting the
PostFeaturedImage
block to have specific height(css) could lead to nice grid of posts/pages etc..Checklist: