Query Loop: Add useBlockPreview, fix Query Loop wide alignment in the editor #36431
Conversation
andrewserong
added
[Type] Bug
|
Size Change: +2.86 kB (0%) Total Size: 1.11 MB
ℹ️ View Unchanged
|
andrewserong
force-pushed
the
fix/query-loop-wide-alignment-in-the-editor
branch
2 times, most recently
from
7dcfacf
to
53fb162
Compare
|
I think this is ready for review now, so I've switched it out of draft. |
andrewserong
force-pushed
the
fix/query-loop-wide-alignment-in-the-editor
branch
from
7c8af49
to
ef7be06
Compare
|
I've just given this a rebase to fix conflicts. |
|
Hi! Thanks so much for putting this together. I did a bit of testing, and it does look to solve the issue! I noticed a bit of changed behavior, though. Whether this is better or worse than the existing align jumping, I don't know -- but I figured it was worth documenting. It's hard to describe, but there are a two things I noticed when selecting / unselecting different posts. I wasn't able to pinpoint the exact situations that cause it, but if I clicked in and out from them couple times, I could usually cause it to happen:
Before: Screen.Recording.2021-11-25.at.18.18.33.movAfter: Screen.Recording.2021-11-25.at.18.15.43.movTested with Firefox 94.0.1 on OSX and Twenty-Twenty Two. |
|
Thanks so much for giving this a test @getsource and including the demo videos, that's super helpful! I don't think I tested this while using a background colour, so didn't notice the flickering issue (nor the scroll issue either 🤔). I'll investigate further (if I don't get time today, I'll follow up next week). Cheers! |
andrewserong
force-pushed
the
fix/query-loop-wide-alignment-in-the-editor
branch
from
ef7be06
to
8a34723
Compare
|
It took me a little longer than expected to fix up the flickering issue. In 8a34723 I've optimised the
I believe this is now working pretty well, but could do with some more testing, as I just got this finished before wrapping up my week. CC: @getsource.
|
andrewserong
force-pushed
the
fix/query-loop-wide-alignment-in-the-editor
branch
from
8a34723
to
cf6b844
Compare
|
This is working as described for me I cannot detect any layout change with selecting inner blocks within the Query Loop block in the post editor.
It's not within the purview of this PR, but I'm not entirely sure that it addresses the Comments Query Loop block issue in #37154
|
|
I can't comment on the implementation in great detail, but, having played with trunk, this is a clear improvement and I'm happy to approve once folks with more high-level context are happy with it. |
|
Thanks for testing @ramonjd!
Yes, the Comments Query Loop block is quite similar so if this PR ends up being accepted, I think we'd then look at a follow-up for that block to deal with that issue 🤞. |
…ing a preview for each block context
Co-authored-by: Ramon <ramonjd@users.noreply.github.com>
andrewserong
force-pushed
the
fix/query-loop-wide-alignment-in-the-editor
branch
from
cf6b844
to
a00533e
Compare
|
Oh! I'm so sorry, I missed the mention for re-testing on this one. It looks like it's been tested now, but I'll take another look on Monday if it hasn't already been merged before then. |
Thanks @getsource! No worries, I've had plenty of other things to keep me busy 😄 It'd be great to get another review/test to confirm the approach as both block previews and the Query Loop block can have quite a few edge cases. Also, since this uses a new hook-based approach for rendering the preview and the disabled behaviour, it'd be good make sure I haven't missed anything from the existing component-based approach, that might be needed here. |
There was a problem hiding this comment.
Okay! I went through and re-tested this, and the flickering seems to be resolved, along with the layout change being fixed.
This is great! In general, everything feels much smoother than pre-PR/trunk.
I don't think I'm the best person for a final code review, so submitting as Comment for that reason. Thanks so much!
There was a problem hiding this comment.
I would be in favor of giving it a try based on the feedback shared during testing. That's the most important aspect of this enhancement. We are planning to recreate a similar set of changes for the Comments Query Loop block as documented in #37154. That should be a good way to validate whether the approach takes scales well.
This PR introduced a new experimental API in @wordpress/components – useDisabled hook. Is that a necessary step at the start or can it be inlined in the @wordpress/block-editor package for now? Do we have more use cases where the same hook could be used? @youknowriad and @ellatrix what do you think about it? If you think it's going to be useful in other places, we should include the documentation so the community knows how to use it.
|
Thanks for testing, folks!
Good question @gziolo! I originally added this hook in a previous PR that didn't land, and over there @youknowriad had the suggestion of placing it in I like the suggestion of adding documentation, Let me know if you'd like further changes! |
That and the suggestion from @youknowriad was the missing bit of information. I think it's a solid plan and with the JSDoc comment included for the new hook, it should be enough to expose the documentation once the API is promoted to stable. I hope you can do it in a follow-up that refactors the |
There was a problem hiding this comment.
Let's get this in. I'm not the best person to approve, but you only live once and I trust all other reviewers 😄
Thank you everyone for an excellent team work to help @andrewserong to take it to the finish line 🎉
Yes, of course! There's a few other tasks to wrap up first, but my goal is to follow-up with a refactor for the Disabled component in the new year. (In the shorter-term, I'd like to continue refining the click behaviour when switching between blocks and block previews in the instances of the loop). Thanks again for the review and merge! |
tellthemachines
removed
the
Backport to WP 6.7 Beta/RC
… editor (#36431) * Add useBlockPreview, fix Query Loop wide alignment in the editor * Add test for useDisabled * Add minimal test for useBlockPreview hook * Add useDisabled test to cover updates to a component * Update useBlockPreview test to ensure elements within the block content are disabled * Switch queryBy to getBy to ensure check for block is stricter * Remove comment * Export useBlockPreview as __experimentalUseBlockPreview * Optimise block preview via memoizing the component, and always rendering a preview for each block context * Tidy up settings useMemo Co-authored-by: Ramon <ramonjd@users.noreply.github.com> * Add documentation for useDisabled hook * Move documentation to the hook Co-authored-by: Ramon <ramonjd@users.noreply.github.com>
|
@andrewserong I don't get at all why we must render all inner blocks twice to fix and alignment issue. Could you try to explain this to me? I don't know anything about this block. |
Thanks for the ping @ellatrix! The only one that gets rendered twice is the current instance of the loop — the double rendering is to avoid flickering when switching between instances. Without it, when clicking between instances, there's a flash of white as the preview renders. If the current instance is rendered for "real" as well as there being a hidden preview version of it, then the preview can be shown / hidden instantaneously. Not the best approach, but resulted in a smoother UI. That could totally be revisited if there are better approaches now, though. For a bit more content: and my memory is a little fuzzy on this particular PR since it's a couple of years old now, but my understanding is that the way that the block works is that it should only render the "real" inner blocks once, for the instance selected. And all other instances of the loop should render a preview instead. If we accidentally render each child as "real" inner blocks (as in #60535) then we wind up with a bunch of duplicate controls in the UI (in both the block toolbar and inspector controls): 
I believe using previews for the non-selected instances of the loop was a workaround for that problem. The previews pre-date this particular PR — this one was an attempt to work around some of the downsides of the preview approach prior to this PR (alignment display, reduce flickering etc). |
Description
This is an alternative to #35932 and is intended to resolve #33248.
It pulls in the
useBlockPreviewanduseDisabledhooks from #35863 and uses them to render a block preview for the Post Template block, for those instances within the query loop that are not currently selected (and are therefore not interactive). By using these hooks, we can avoid adding the DOM nesting involved in using theBlockPreviewcomponent directly.The
useDisabledhook is mostly comprised of logic borrowed from the Disabled component. The context behaviour has not been included in the hook version, because we don't (yet) have a need for it. In a future follow-up, we could refactor theDisabledcomponent to use theuseDisabledhook introduced here, internally.How has this been tested?
In a page, insert a Query Loop block and set the alignment to Wide and make sure that the number of items within the loop is at least 2-3. Within the query loop block, select between the instances within the loop. Unlike in the reported issue (#33248), selecting between blocks should preserve the Wide layout.
Note: this is specifically about dealing with adding a Query Loop block to a page — behaviour in the site editor should be unaffected
Screenshots
Types of changes
Bug fix (non-breaking change which fixes an issue)
Checklist:
*.native.jsfiles for terms that need renaming or removal).