Consolidate and resolve display issues between InserterPreviewPanel and BlockStylesPreviewPanel #56011
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
richtabor
added
[Type] Bug
|
Size Change: -61 B (0%) Total Size: 1.72 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in 76dc08c. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/6964430977
|
|
I'm tempted to green check this simply because it is a reduction in code and therefore edgecases, and is in most cases a simplification. Removing the duplicate shadow is definitely also a necessary bugfix. But I'm waffling a bit on the removal of the edge-to-edge code that makes the preview spand the full width of the popover. It's both less space available for the preview, but it's also a little character that gets lost. Though admittedly that character is less effective in practice than in theory, I find the borders around the popovers get a bit heavy sometimes, and when they are edge to edge with a preview it sometimes clashes. It just isn't clear either, that borderless popovers would be a win. |
This is the main reason I pushed for it. Typically, if there is a background color associated with the preview, it'll look more like these screenshots below. Space wise, I'm not so concerned, as the scaling is just about the same.
|
chad1008
reviewed
Nov 13, 2023
There was a problem hiding this comment.
Code looks good, and tests well for me. I don't have a strong opinion on the edge-to-edge convo above, so I'll let @jasmussen drop a green check when y'all are happy with the decision there.
|
I'm happy to try and ship this, we can always revisit holistically. That said, I think it's on Jay's mind to revisit elevation and shadows, if we find a good solution there, that might provide enough bounds with surrounding content that we don't need the border. |
I quite like the spacing here. With the scaling and clearer separation from the canvas it make the preview feel a bit more distinct. |
|
Thanks team. I'll rebase and mark it for review. 🚀 |
richtabor
force-pushed
the
fix/block-styles-inserter-popover
branch
from
e798b73
to
76dc08c
Compare
|
@jasmussen @jameskoster I rebased it, if you could give it a final spin/review. Thanks! |
jameskoster
approved these changes
Nov 23, 2023
richtabor
force-pushed
the
fix/block-styles-inserter-popover
branch
from
76dc08c
to
a80a001
Compare
richtabor
force-pushed
the
fix/block-styles-inserter-popover
branch
from
a80a001
to
f0b7645
Compare
|
Tests are passing; I'll merge. Thanks! |
t-hamano
reviewed
Dec 6, 2023
| @@ -7,6 +7,7 @@ | |||
| // The preview component measures the pixel width of this item, so as to calculate the scale factor. | |||
| // But without this baseline width, it collapses to 0. | |||
| width: 100%; | |||
| height: 100%; | |||
There was a problem hiding this comment.
This style seems to affect the layout of the pattern in the inserter. The problem is summarized in #56812. We may want to apply this style only to a single block preview.
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?
I noticed the BlockStylesPreviewPanel preview popover had duplicative "popover" styling around it (double borders/shadows, as noted in the "before" screenshot below).
It occurs because the
InserterPreviewPaneldoes not use a popover, whereas theBlockStylesPreviewPaneldid. So I updatedInserterPreviewPanelto use a popover in the same fashion, so that it would be consistent with the BlockStylesPreviewPanel—and both receive the same design treatments.I was then able to reduce some of the overrides/duplicative styling, while also styling the popovers to look like they do elsewhere. I'd like to follow-up with using the global styles background color as the BlockPreview background color perhaps, which would fill in the preview area better.
Testing Instructions
Screenshots or screencast