[Block Library - Query]: Add layout support and rename previous layout prop to displayLayout
#31833
Conversation
ntsekouras
added
[Type] Enhancement
ntsekouras
requested review from
mcsf,
youknowriad,
mtias,
aristath,
gziolo,
jameskoster,
jasmussen and
a team
ntsekouras
requested review from
nerrad,
ntwb,
spacedmonkey and
TimothyBJacobs
as code owners
|
Size Change: +241 B (0%) Total Size: 1.62 MB
ℹ️ View Unchanged
|
ntsekouras
force-pushed
the
add/query-layout-support
branch
from
142e32b
to
050d587
Compare
|
This is working well for me. In terms of changes, here's a basic before:
And after:
Ultimately, I feel like this is fine to do, since we've already decided to add a div to the loop. That also, in my opinion, reduces the risk of this one. Does that make sense? Finally, I see no difference in the block based themes I tested. So seems fine to add this. |
|
I have a couple questions/observations:
|
|
I'm seeing the layout toggle on the Query block, and the alignment controls on the pagination and query loop. So I guess the PR is working as advertised. I do find these controls incredibly confusing to use though. Sometimes the wide and full-width options seem to do the same thing, regardless of the layout toggle on the parent. Also, when the parent doesn't inherit the layout then the alignment option on the pagination block disappears, but remains in the query loop. layout.mp4I am not too familiar with this control, so perhaps this is intended. If so I feel the design needs some love. Edit: Also worth noting that I'm doing this in the site editor, maybe this isn't optimised for that environment yet? |
It's the same for
This is expected as |
I'm just referring to the block UI here, not the patterns themselves (though it definitely does get confusing when viewing the carousel 😅). What I'm observing is that for the group block, when you select a layout, the block's UI (in this case, the appender) has no change:
... but for the Query block, the block's UI (the pattern selector) gets smaller:
Is that expected? It feels wrong to me, though I'll admit I'm still feeling a little lost about this feature. |
|
I agree that it's better if it the placeholder stays full wide, not sure how easy to do with our current way to output layout styles. |
This is more related to the below comment. The best thing would be not to be able to change alignments and other props while seeing the Placeholder.
@youknowriad a quick test from you would be great though due to all your |
Yeah, I'm seeing another bug: when I set layout in the placeholder state, they get cleared after I hit "choose":
That would presumably go away if we didn't let people choose layouts at the placeholder state. |
This is the issue I'm talking about:
|
|
Did we remove the "layout" support from query loop? |
|
Late, but I am able to reproduce what Kjellr is mentioning. |
|
I think the bug mentioned by @kjellr is independent of this PR, it has to do with how "pattern transforms" work, right? |
Yes, It has to do with the pattern suggestion - experimental |
|
For what it's worth, this PR is not working for me at this point. I'm curious if others are seeing these same issues. Here's a quick walkthrough (with audio): Screen.Recording.2021-05-19.at.8.49.17.AM.mov |
That's not what the description of the PR suggests :) |
I typed the wrong thing... It was meant to be |
|
It seems this PR works as intended for me then. I think classic themes will have trouble providing the CSS styles for these blocks though without shifting to theme.json + layout (just like they have trouble now getting group blocks to work well with alignments) |
ntsekouras
force-pushed
the
add/query-layout-support
branch
from
85141c2
to
bc70ade
Compare
ntsekouras
force-pushed
the
add/query-layout-support
branch
from
bc70ade
to
594084b
Compare
Yeah, I hadn't noted mentioned that issue specifically in the video, but I can confirm that it is fixed now. The other issues in the video are still valid in my testing after merge — I'm still not seeing this PR actually work the way I'd expect. Most prominently: when I activate the layout controls on the Query block, the wide/full child blocks don't appear wide or full, and do not have access to wide/full alignment controls at all:
Are other folks encountering this? My understanding was that this PR was meant to enable that. Otherwise I'm not sure what the layout control is supposed to do here. |
|
That's because there's another level of nesting. In other words, the wide/full should appear in the "Query Loop" block but not in its content. I guess the "layout" support could be added to the "QueryLoop" block separately as well. |
|
Yeah... I thought this would enable it for both (probably due to the same oversight noted here). From my experience designing these patterns, I think having these controls available for the Query Loop block will be far more useful than having them on the Query block. On a related note, I definitely worry about overcomplicating these interactions... I've found it really confusing to nest layout controls like this. But we can discuss that separately. |
Description
Closes: #31621
This PR does a couple of things regarding
Queryblock and its 'basic' childrenQueryLoopandQueryPagination.layoutandcolorsupport toQueryblock to better handle the layout/alignments of the blocks and remove the need to wrap it inGroupblock.alignsupport toQueryLoopandcolorsupport toQueryPaginationblock.layoutproperty todisplayLayoutbecauselayoutis used for the layout support.Notes
I have added a filter to
Query editin order to show thecreate new postlink at the top of theInspector controls. The order of InspectorControl children is based on the render order and with the addition of the new supports(color, layout) it was not displayed properly. The proper solution will be with an implementation of grouping these controls like the recent change inBlockControlswith thegroupkey.Testing instructions
Queryblock and test different alignmentsTypes of changes
(**experimental breaking **)
This change will make existing
Queryblocks withflexlayout (columns view) not to display as it was before, but as a vertical list. I couldn't avoid this because this property was passed on and used fromcontext. That makes it difficult to handle this with a deprecation because it was used in server-side (index.php) as well..I believe it's okay to have this change in order to have this in
corefor 5.8 as it will be much more difficult to handle when it lands there. We've made some more similar changes to other new blocks likeSiteLogo.Screenshots
Checklist:
*.native.jsfiles for terms that need renaming or removal).