Block toolbar: restrict visible child calculation to known blocks #66702
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…pecific blocks that we know overflow. Rename function and update comments. Introduce rudimentary unit tests.
6 tasks
ramonjd
commented
Nov 3, 2024
|
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 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. |
|
Size Change: +34 B (0%) Total Size: 1.81 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
Thanks for continuing to narrow in on solution to the toolbar positioning @ramonjd 👍
This tests well for me:
✅ Toolbar adjusts as expected for Nav block when it is the top-most block
✅ Toolbar is relocated when expanding Nav block submenus, adding items etc.
✅ Toolbar is correctly positioned when working with scrollable blocks
✅ No regressions around block popovers and visualizers
In general, this is looking good to me.
These tests are basic so far. I'll flesh them out, but they provide a small sanity check.
Sounds good. With a few extra tests, I think this is pretty close.
Testing nav block in top-most position
Screen.Recording.2024-11-04.at.10.27.20.am.mp4
Testing scrollable blocks
Screen.Recording.2024-11-04.at.10.29.50.am.mp4
Testing block popover visualizers
Screen.Recording.2024-11-04.at.10.32.41.am.mp4
Co-authored-by: Aaron Robertshaw <[email protected]>
…strict-blocks' into update/dom-get-element-bounds-restrict-blocks
There was a problem hiding this comment.
Thanks for the quick turnaround on the extra tests 🚀
The coverage is looking a lot better. Another nice addition could be some tests for viewport boundaries and blocks that might partially lie outside them etc.
Other than that, I left a few minor thought bubbles as inline comments around test descriptions.
|
Thanks for the suggestions @aaronrobertshaw All implemented in ae72973 |
aaronrobertshaw
approved these changes
Nov 4, 2024
2 tasks
t-hamano
approved these changes
Nov 4, 2024
There was a problem hiding this comment.
LGTM! I have tested five plugins that have scrollable elements and they all seem to work correctly.
- Advanced Columns Block
- Piano Block
- Custom HTML Block Extension
- The Icon Block
- WordPress Slider Block Gutenslider
2d0f6690e00d624d7a4dba9037c8ab00.mp4
|
Would it be possible to backport this PR and #66546 into a minor release? |
I'll add it to the 6.7.1 column in the project board 👍🏻 Thanks for testing so thoroughly, everyone! |
getdave
reviewed
Nov 5, 2024
| @@ -134,41 +134,47 @@ function isScrollable( element ) { | |||
| ); | |||
| } | |||
|
|
|||
| export const WITH_OVERFLOW_ELEMENT_BLOCKS = [ 'core/navigation' ]; | |||
There was a problem hiding this comment.
Not sure if you considered this elsewhere, but I wonder whether in the future we could make this list extensible as an editor setting? That way it can be modified in PHP for custom blocks?
There was a problem hiding this comment.
Thank you for cherry picking this one!
@t-hamano had an idea to add it to the block api
I think it's worth investigating if it the issue arises again.
getdave
added
the
Backport to WP 6.7 Beta/RC
github-actions
bot
removed
the
Backport to WP 6.7 Beta/RC
gutenbergplugin
pushed a commit
that referenced
this pull request
Nov 5, 2024
…6702) * Refactor getVisibleElementBounds to only check visible children for specific blocks that we know overflow. Rename function and update comments. Introduce rudimentary unit tests. * Apply suggestions to code comments from code review Co-authored-by: Aaron Robertshaw <[email protected]> * Add extra tests * Rebase * Added a test for viewport clipping and adjusted test descriptions --------- Co-authored-by: ramonjd <[email protected]> Co-authored-by: aaronrobertshaw <[email protected]> Co-authored-by: t-hamano <[email protected]> Co-authored-by: ndiego <[email protected]> Co-authored-by: getdave <[email protected]> Co-authored-by: stokesman <[email protected]> Co-authored-by: andrewserong <[email protected]> Co-authored-by: simom <[email protected]>
github-actions
bot
added
the
Backported to WP Core
|
I just cherry-picked this PR to the wp/6.7 branch to get it included in the next release: 02da42e |
karthick-murugan
pushed a commit
to karthick-murugan/gutenberg
that referenced
this pull request
Nov 13, 2024
…rdPress#66702) * Refactor getVisibleElementBounds to only check visible children for specific blocks that we know overflow. Rename function and update comments. Introduce rudimentary unit tests. * Apply suggestions to code comments from code review Co-authored-by: Aaron Robertshaw <[email protected]> * Add extra tests * Rebase * Added a test for viewport clipping and adjusted test descriptions --------- Co-authored-by: ramonjd <[email protected]> Co-authored-by: aaronrobertshaw <[email protected]> Co-authored-by: t-hamano <[email protected]> Co-authored-by: ndiego <[email protected]> Co-authored-by: getdave <[email protected]> Co-authored-by: stokesman <[email protected]> Co-authored-by: andrewserong <[email protected]> Co-authored-by: simom <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes: #66438
What?
Refactor
getVisibleElementBoundsto only check visible children for specific blocks that we know overflow. Rename function and update comments. Introduce rudimentary unit tests.Props to @t-hamano and @stokesman for the ideas and testing.
Why?
The positioning logic was rolled out to the general block population in #62711 when the only use case was the navigation block. This hasn't changed.
See discussion: #66438 (comment)
How?
Implementing ideas discussed in #66438 (comment)
Testing Instructions
Navigation block
Scrollable block toolbars
Follow the nice and detailed instructions on #66112
TL;DR
Block Popovers and Visualizers
npm run test:unit packages/block-editor/src/utils/test/dom.jsTest coverage is basic so far, just for sanity checking purposes.