Cover: Add border support using ResizableBox via BlockPopover #41153
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
aaronrobertshaw
added
[Type] Enhancement
This was referenced May 19, 2022
|
Size Change: +412 B (0%) Total Size: 1.34 MB
ℹ️ View Unchanged
|
aaronrobertshaw
force-pushed
the
try/resizable-box-popover-for-cover-block
branch
from
c8ee552 to
3256667
Compare
aaronrobertshaw
force-pushed
the
try/resizable-box-popover-for-cover-block
branch
from
3256667 to
fa99bae
Compare
aaronrobertshaw
force-pushed
the
try/resizable-box-popover-for-cover-block
branch
from
fa99bae to
2ba4f87
Compare
1 task
6 tasks
aaronrobertshaw
force-pushed
the
try/resizable-box-popover-for-cover-block
branch
from
2ba4f87 to
523c716
Compare
aaronrobertshaw
force-pushed
the
try/resizable-box-popover-for-cover-block
branch
from
523c716 to
49b7d18
Compare
|
Flaky tests detected in 492e099. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4506269124
|
aaronrobertshaw
changed the title
|
The issue with the block popover not staying over the block while scrolling in the editor has been resolved. As such, I've rebased this PR to pick up the fix. I'll give this a further test and polish tomorrow but it should finally be close to reviewable. |
aaronrobertshaw
force-pushed
the
try/resizable-box-popover-for-cover-block
branch
2 times, most recently
from
780b640 to
ef2798b
Compare
|
There are still a few small issues around the resizable box here. While they aren't show-stoppers, it would be best to get them ironed out before investing any time into reviews here. |
|
Most of the kinks have been ironed out of this now. The drag resizing could be a little smoother but now might be a good time to get some fresh eyes and testing on this approach. |
The issue that @talldan had with the drag cursor that I mentioned was different to the issue noted here it turns out.
At a quick look I think to fix this we would need to get the computed height of the current block and limit the resizable box height to that - but the placeholder height doesn't necessarily relate to the final min height a person may want - so maybe we only do this if not the placeholder? |
I've pushed a commit that fixes this. Since the What was happening is that when moving your mouse quickly, the cursor can end up over the editor canvas iframe (instead of over the drag handle). The iframe intercepts The fix I've applied is to ensure It might be good to get a check from @ellatrix whether this is ok as a fix, as there's a comment about removing iframe event bubbling. The alternative fix would be to avoid using a popover and instead find a way to offset the ResizableBox's position by the border amount. |
|
Appreciate the fix @talldan! Bonus points for the explanation as well 🙇
I have vague recollections about exploring this previously while working on the other failed PRs linked in this PR's description. I didn't have much success there although I don't recall the exact reasons why off the top of my head. They were a long time ago. One benefit of the ResizableBox in the block popover is that it prevents the ResizableBox from causing disparity in block markup between the editor and frontend. 🤞 we can proceed with the current approach as I think it's finally in a good place. Thanks again for the fix! |
Any estimate when this will arrive to the Gutenberg plugin or WordPress Core respectively? |
|
Thanks for the question @porg 👍
We still need to ensure that the fix that allows smooth drag resizing of the Cover block (564fd60) is acceptable. If it is, the plan is to get this one merged and into the following Gutenberg release. I'd then expect it to be in the 6.3 WordPress release. |
aaronrobertshaw
force-pushed
the
try/resizable-box-popover-for-cover-block
branch
from
564fd60 to
492e099
Compare
|
@aaronrobertshaw Thanks for providing an estimate and timeline! Looking forward to getting this! |
talldan
approved these changes
Mar 31, 2023
|
Looks like this broke the recently added block unit tests for Cover on trunk. I'm investigating now with the view to fix asap. |
|
Fix for failing Cover test is up in #49497 |
6 tasks
12 tasks
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.
Issue:
Related:
What?
ResizableBoxwithin aBlockPopoverWhy?
This much-requested feature will unlock a lot of potential new designs.
How?
color,radius,style, andwidthResizableBoxPopoverto facilitate rendering aResizableBoxvia aBlockPopoverResizableBoxPopoverin Cover block to avoid borders conflicting with height adjustmentsKnown Issues
Testing Instructions
npm run test:unit packages/block-library/src/covercore/coverblock.Screenshots or screencast
Screen.Recording.2023-03-10.at.6.17.32.pm.mp4