Fix padding/margin visualizer placement #44484
Merged
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
talldan
added
[Type] Regression
|
Size Change: +10 B (0%) Total Size: 1.26 MB
ℹ️ View Unchanged
|
aaronrobertshaw
approved these changes
Sep 27, 2022
There was a problem hiding this comment.
Thanks for the fix @talldan 👍
✅ Could replicate the issue on trunk
✅ Block popover now correctly overlays block (when it is fully in the viewport)
Click for before/after demos
| Before | After |
|---|---|
Screen.Recording.2022-09-27.at.4.37.54.pm.mp4 |
Screen.Recording.2022-09-27.at.4.38.33.pm.mp4 |
The only issue I found while testing was that if the block is partially out of the viewport, the block popover is forced completely within the viewport. This is the behaviour that was present on trunk before #44323 though, so I don't think it should block this fix.
Video of above issue
Screen.Recording.2022-09-27.at.4.41.42.pm.mp4
|
Thank you for working on this one! In debugging a margin visualization regression, I also happened upon #44323, which led me to this (cc: @ciampo). Just noting that this PR fixes the padding, but there still appears to be an issue with the margin visualization. Before 44323: After 44323:
Trunk (after this PR):
|
@jasmussen I had trouble reproducing that one at the time (as mentioned here - #44335 (comment)), but I see it now. I checked and it doesn't look like it was caused by #44323 (I checked out 5359314, which is the commit to trunk prior to that PR being merged). That makes sense because this PR effectively reverts the changes that caused the issue in #44323. There's also a separate bug fix for the visualizers in #44485. I had to split it into two PRs as only one needed to be backported. It's likely that the regression that PR fixed is more related to what causes the margin issue, though I can't be sure. |
I can't speak with much confidence as my skill in finding the source is very limited. I essentially go to https://github.com/WordPress/gutenberg/commits/trunk and check out a much older iteration and see if it works, then step forward until I find a PR that breaks it. In my case, it consistently worked in 5359314 and broke in 463ce23. However I did have to do a slew of npm install's and ci's in between, so there's every chance I had a ghost in the machine. |
|
I tested again and I'm seeing the same results as I did before. That PR didn't introduce the regression. Whether the bug happens does seem dependent on which block is being tested. For example If I test the group block within the heading in TT2, it seems to work fine, even on If I test the group block within the heading on TT3, I'm seeing the bug you mention. It may well be that this issue is only exposed by a combination of block styles, which may make it hard to figure out when it was introduced. It could even be quite an old bug, but it's only become noticeable since TT3 became default. I'd suggest making a new issue for it, as it looks like a separate bug to the two other issues that were fixed. |
|
Thank you. I created #44693. |
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.
What?
Fixes the issue noted here - #44323 (comment), and here - #44335 (comment)
After a recent refactor of the
BlockPopovercomponent, the padding/marging visualizers were incorrectly positioned below the block.Why?
The
position/placementprop for those components was accidentally removed.How?
Adds the
placementprop toBlockPopoverso that all components that render it default to a 'top-start' placement.Testing Instructions
Screenshots or screencast
Before
After