Query block: move patterns modal to dropdown on block toolbar #66993
Conversation
|
Size Change: +533 B (+0.03%) Total Size: 1.82 MB
ℹ️ View Unchanged
|
|
Nice one! This is working as intended:
I can't seem to click this "Choose" button, though, it appears to do nothing for me:
Also a separate note: it would be nice if there was a way for this design panel to start out uncollapsed:
It's probably closed because of an issue loading the previews, you think? |
|
Thanks for the fast review! It's not quite ready yet, just a shell of a PR 😄
Oh, that should open the modal right? I think I unhooked that completely by accident. I'll have to reinstate.
It's possible as far I know. I left it collapsed because the list is very looooong. But I guess we could overflow-y + scroll? What do you think? Once I've tidied things up I'll add a better test description 😄 |
It does, yes. |
It's not for this PR; it's a question that's been on my mind also for templates. The contents of that Design panel is hugely valuable, so the fact that it's closed reduces prominence that would otherwise be good to have. If we were able to open it (again, probably best to keep that separate and do that across all design panels), the sidebar already scrolls, so perhaps the solution to it being long is just to keep the Design panel as the last one (just before Advanced), so it doesn't push down other controls. |
ramonjd
force-pushed
the
update/query-block-add-design-to-inspector-tools
branch
from
6407a2f
to
d7839c7
Compare
ramonjd
added
[Block] Query Loop
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the Unlinked AccountsThe following contributors have not linked their GitHub and WordPress.org accounts: @jarekmorawski. Contributors, please read how to link your accounts to ensure your work is properly credited in WordPress releases. If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
Thanks for the feedback. I've left it open for now. Let me know what you think. Once thing I changed about the block inspector patterns list was to show tooltips rather than the titles, which normally appear underneath the images. I found the titles personally confusing, and wasn't sure which pattern they referred to in a long vertical list. 
Can go either way on it. I'll take your advice on that one. |
packages/block-library/src/query/edit/inspector-controls/index.js
Outdated
Show resolved
Hide resolved
|
Thanks for the PR! If I remember correctly we have the Despite where we end up (closed/opened) it seems to me it fits better below the main block's control. Finally, this implementation for Query Loop has also a |
ramonjd
force-pushed
the
update/query-block-add-design-to-inspector-tools
branch
from
620981d
to
5629744
Compare
|
Thank you for helping here!
Good point. I had it closed initially because of this concern. The patterns load with the site editor so at least there's not an extra API call. I'm not sure to be honest. The list of patterns has the potential to be very long. Would it help or annoy users to have it permanently open? If I'm tweaking my query block, I would personally find it a bit of a peeve to have to close it to access my query controls.
I had a little look at it appears the layout inspector control has priority, so I guess — I'm not 100% on this yet — we'd need to specify a new group and order it explicitly in the block inspector component. Looking at this, I think there's an argument for having it lower - the query controls are quite useful and the design panel, if left open, has the potential to be very long. 
🤷🏻
I left the search control in for no reason in particular. I think it's only useful if you know what you're looking for. The list can be long, so I guess it's useful for theme devs with knowledge of the theme. For this PR, I could remove it if folks don't feel strongly about it. Or, if it goes the other way, it could be added across the other design panels in another PR. |
|
Putting this in draft for a while while I test some things. I was chatting with @annezazu about it and it was important to show something in navigation mode (write mode). See: #66614 So I'm just hacking about to see if it's possible to create a Conceptually it works, but it probably breaks a lot of things too. @ntsekouras not a priority, but when you get a chance, what do you think about the requirements? Is there a better way to display controls in the inspector toolbar in write mode? Kapture.2024-11-21.at.15.49.50.mp4Thank you! |
| @@ -276,7 +281,7 @@ const BlockInspectorSingleBlock = ( { | |||
| </PanelBody> | |||
| ) } | |||
|
|
|||
| { ! isSectionBlock && ( | |||
| { ! isSectionBlock && ! isNavigationMode && ( | |||
There was a problem hiding this comment.
Are these tests the same? When could they be different?
Here's the selector:
There was a problem hiding this comment.
There is an algorithm to try to determine what a section block is and has to do with zoom-out as well. @getdave might have more info, but I don't think it's a check we should add.
There was a problem hiding this comment.
Maybe we should not do it in write mode and instead do it for the block in design mode 1st. I think content only representation of blocks that do not expose role content attributes is a larger thing to consider. There is at least #65699 to consider as well
| @@ -309,6 +314,10 @@ const BlockInspectorSingleBlock = ( { | |||
| </div> | |||
| </> | |||
| ) } | |||
|
|
|||
| { isNavigationMode && ( | |||
There was a problem hiding this comment.
Basically what I'm trying to achieve here is a slot for tools that should only appear in write mode.
There was a problem hiding this comment.
@youknowriad @ramonjd here is basically trying the original idea I had too for #66753 have a slot where blocks can put things in certain edit modes. The query block puts a design picker, the navigation block shows the list view. But I believe you had some objections?
It seems to be a problem that it does add yet another slot and we're unclear what "content only representation" even means and we rush into something public.
| @@ -70,6 +71,7 @@ export default function QueryContent( { | |||
| ?.posts_per_page; | |||
|
|
|||
| return { | |||
| isNavigationMode: _isNavigationMode(), | |||
There was a problem hiding this comment.
Not sure if this is best way to test for write mode inside a block?
Or should we be rather testing for isSectionBlock?
There was a problem hiding this comment.
It's important to figure out if blocks should check for that. It would be nice if the block editor should know what to do based on something about these blocks (what does navigation and query block have in common...).
Write mode is a top level content only lock, and blocks do have content role attributes. They should not have to implement special UI to "support" features of the block editor, if they support it the UI works.
|
Thanks for exploring this! I don't know in depth all the nuances with all the different modes (editorMode, blockEditingMode, etc..) so I'm not sure I can give good feedback. I'll cc @WordPress/gutenberg-core . My thoughts would be that for this Query Loop issue, I imagine changes in how we handle blocks with inner blocks in Having a separate slot could work I guess and we would also need slot for block Toolbar, but right now we have some checks in |
There was a problem hiding this comment.
@WordPress/gutenberg-design should we have a unified approach to write mode for blocks that "should work" in write mode but have no content exposed, or is a block by block approach needed?
@ramonjd could we make the PR address design mode 1st?
| @@ -276,7 +281,7 @@ const BlockInspectorSingleBlock = ( { | |||
| </PanelBody> | |||
| ) } | |||
|
|
|||
| { ! isSectionBlock && ( | |||
| { ! isSectionBlock && ! isNavigationMode && ( | |||
There was a problem hiding this comment.
Maybe we should not do it in write mode and instead do it for the block in design mode 1st. I think content only representation of blocks that do not expose role content attributes is a larger thing to consider. There is at least #65699 to consider as well
| @@ -309,6 +314,10 @@ const BlockInspectorSingleBlock = ( { | |||
| </div> | |||
| </> | |||
| ) } | |||
|
|
|||
| { isNavigationMode && ( | |||
There was a problem hiding this comment.
@youknowriad @ramonjd here is basically trying the original idea I had too for #66753 have a slot where blocks can put things in certain edit modes. The query block puts a design picker, the navigation block shows the list view. But I believe you had some objections?
It seems to be a problem that it does add yet another slot and we're unclear what "content only representation" even means and we rush into something public.
| @@ -70,6 +71,7 @@ export default function QueryContent( { | |||
| ?.posts_per_page; | |||
|
|
|||
| return { | |||
| isNavigationMode: _isNavigationMode(), | |||
There was a problem hiding this comment.
It's important to figure out if blocks should check for that. It would be nice if the block editor should know what to do based on something about these blocks (what does navigation and query block have in common...).
Write mode is a top level content only lock, and blocks do have content role attributes. They should not have to implement special UI to "support" features of the block editor, if they support it the UI works.
| @@ -49,6 +49,12 @@ | |||
| } | |||
| } | |||
|
|
|||
| .block-library-query-toolspanel__design { | |||
| .block-library-query-pattern__selection-content { | |||
| margin-top: $grid-unit-10; | |||
There was a problem hiding this comment.
Is this an override of sorts?
There was a problem hiding this comment.
It's to compensate for the removal of the search component in the patterns view.
| @@ -152,6 +154,24 @@ export default function QueryContent( { | |||
| setAttributes={ setAttributes } | |||
| clientId={ clientId } | |||
| /> | |||
| { hasPatterns && ( | |||
| <InspectorControls | |||
| group={ isNavigationMode ? 'navigationMode' : 'default' } | |||
There was a problem hiding this comment.
Maybe we shouldn't need this bifurcation and the block editor should know how to handle write mode? The more I say it the less I think it's doable. Even here, query block shows a design picker, navigation block shows a tree of inner blocks.
|
Would it make sense to make 
|
|
Funny you should mention it, I had a similar thought this morning:
Intoducing a new tab is something I'd like to move a bit carefully with, in a Plan B kind of sense, because doing so introduces a few variables in terms of discoverability of the other settings. It may work! But it would be good to improve some of the other basics first. E.g. performance if there's anything to get, opening by default, a consistent position (always top, or always bottom), starting out open if possible, addressing the jumpiness on loading, perhaps with animation or skeleton indicators, or even fixed aspect ratios (3:4 for pages, 4:3 for patterns perhaps). Those improvements might still benefit anything in a tab. |
I'd really like us to do so too. IMO I lean towards implementing what we did with zoom out here if we can as adding the "design" panel to the sidebar for the query loop block comes with many downsides:
I defer to design here but this is what's on my mind. |
ramonjd
force-pushed
the
update/query-block-add-design-to-inspector-tools
branch
from
92d4c8d
to
436c9b5
Compare
Thanks @annezazu and to everyone for their feedback. You make a pretty good case in relation to the downsides. I don't think the block inspector sidebar has the real estate. I've a mind to close this PR. Still, I've updated this PR to reflect the [original issue's](Resolves #63497) intentions for the sake of testing. Happy to close this one until a consistent write-only state for block tools surfaces.
Another question could be: are there any blocks that require tools in write mode that are currently blocked by the If there's a comfortable home for designs in the toolbar, and no further requirements, then maybe this PR can be abandoned. |
I think block style variations are shown in the inspector for sections when zoom out is active, possibly also in write mode. Some blocks also have special toolbar controls, but I think that's just handled within the block via some conditional rendering (image block with its alt text is an example). |
There's some evolving work happening over on #67140 to add block style variation controls to the block toolbar in zoom out mode. |
I think that condition is not "final", in the sense that it's a temporary solution to the fact that we want to hide most block tools in these modes (content-only, write mode, zoom-out) and instead we only show the tools that the framework (block-editor) can provide and not the ones that the block provides. I agree that it has proved to be a challenge. What's clear (from multiple PRs) is that we need to come up with a good system to decide:
|
|
Quickly tested this and noticed that I can't find the write/design tools in the PR now. I can see a collapsible design section under the Query Loop though but worry this might have messed with something else. missing.tools.mov |
I think both affordances are appropriate, but perhaps we clean up to where we are already headed today (the "Change design" toolbar control)—before adding something new.
For blocks with alternate patterns defined via block types, like Query, Header, and Footer, I think it's fine to have the "Change design" control available anytime. It's unlikely to be available in zoom out, unless it's also a top-level block. |
@annezazu The write mode was placed behind an experimental flag in #67008 
Could that be it? |
|
I like the veriation @richtabor showcases in the toolbar much better than the version in the sidebar. The sidebar just takes up too much vertical space and only having a single pattern up at a time makes it very hard fo actually find / compare anything. |
Yes! I am a fool and didn't connect that that happened just before this :D |
ramonjd
force-pushed
the
update/query-block-add-design-to-inspector-tools
branch
from
436c9b5
to
87b24ab
Compare
I've done this for the query block. 👍🏻 It's a first pass so if you can see opportunities for refinements, let me know. Thank you! |
packages/block-library/src/query/edit/inspector-controls/index.js
Outdated
Show resolved
Hide resolved
ramonjd
force-pushed
the
update/query-block-add-design-to-inspector-tools
branch
from
6e884dc
to
5797f50
Compare
ramonjd
changed the title
There was a problem hiding this comment.
Thanks for all the iterations here @ramonjd 👍
This is testing well for me.
✅ Patterns are rendered appropriately via "Change design" in the block toolbar
✅ Replacing patterns works as expected
✅ Change design popover renders and works in mobile view
Screen.Recording.2024-11-27.at.12.12.24.pm.mp4
My understanding is you've addressed all the feedback for this first pass, so to that end I'm happy to approve. Feel free to wait for a second opinion or design approval if you'd like though.
|
Thanks a lot for testing @aaronrobertshaw!! 🙇🏻 I'll leave this one to simmer overnight to see if design folks suggest any last-minute tweaks. |
| </ToolbarGroup> | ||
| ) } | ||
| </> | ||
| <ToolbarGroup className="wp-block-template-part__block-control-group"> |
There was a problem hiding this comment.
Not for this PR but how much of this toolbar control can be made generic and absorbed automatically for all blocks in "block-editor"? (just asking whether that's even possible)
There was a problem hiding this comment.
I had a poke around and I think it could be genericized in the block-editor package.
For example the align hook displays a block control.
As far as I can tell the hasPatterns check the query block uses relies only on the blocks and block-editor stores. So no core data.
What?
Resolves #63497
This PR:
How?
Extracts the belly from the modal to make it reusable.
Also abstracts the
hasPatternstest for consistency.Testing Instructions
Screenshots or screencast
Kapture.2024-11-27.at.10.14.24.mp4