Style Book: Add extra margin to support displaying box shadow #48470
Conversation
andrewserong
added
[Type] Bug
andrewserong
requested review from
apeatling,
madhusudhand and
tellthemachines
|
I'm not too sure which designers were involved in the original styling of the Style Book, so please excuse the wider ping! @WordPress/gutenberg-design for visibility — very happy for feedback or other ideas on how to approach this if there's a better way to do it 🙂 |
|
Size Change: +664 B (0%) Total Size: 1.33 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in 3249e1c. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4279358856
|
| '.wp-block:first-child { margin-top: 0; }' + | ||
| '.wp-block:last-child { margin-bottom: 0; }', | ||
| '.is-root-container > .wp-block:first-child { margin-top: 16px; }' + | ||
| '.is-root-container > .wp-block:last-child { margin-bottom: 16px; }', |
There was a problem hiding this comment.
I verified the changes with different blocks.
Since shadow can grow in any direction, it needs space in all four directions.
I think it is best to set margin for .is-root-container {margin: 1rem}
I guess a negative margin for edit-site-style-book__example is required to reset the space.
To me, the ideal would be for the previews to have a visible overflow, but the previews are in an iframe where Part of the issue in the style book is that the blocks feel like they're on a canvas, so the cropping is unexpected, it's a bit different to the global styles sidebar where the previews are more clearly boxed in: The inserter preview could be a bit better, not sure why there's so much empty space: I think you're taking the right approach in improving the situation, I can't think of any obvious alternatives. |
|
Just to echo Dan, the margin is fine but might not solve the problem for a sufficiently big shadow. Can we just allow visible overflow on the preview container in this particular case? |
|
There's a small chance that a visible overflow could result in obscured UI if an extreme shadow is used. Or overlaps when consecutive blocks have shadows. Keeping the hidden overflow but adding some padding and a border helps you to understand why the shadow is cropped without jeopardising the rest of the UI. It's not a strong opinion but to be safe I lean slightly in that direction.
|
|
All of the shadows worked except for the largest shadow. It did strike me as odd being cut off since in the stylebook you'd expect it to look how it's going to look on the site.
|
|
Thanks for all the feedback, everyone, lots of good notes! The cutoff of larger shadows is a limitation of using a hard-coded margin like this, but was one way to avoid a more complex approach. I was keen to see if a more complex approach could work, so I've opened up an alternative PR over in #48578 that attempts to extract the real value for the shadow and use it to general margin rules for the preview. It's quite a lot more complicated, but seems to work a bit more reliably. I'm not too sure if I'm sold on adding all the extra complexity, but I'm happy to keep progressing with that PR if folks would prefer that approach instead 🙂 Now, back to this PR —
Unfortunately we can't as the shadow is rendered within the preview
I really liked this idea, but once I started adjusting background colours in my theme, the additional border around the preview seemed to remove the nice WYSIWYG feeling of the style book for me. For example, when playing around with a dark colour scheme, it looked like this:
So, I'm not too sure if the added border will really work, but again, happy to explore other ideas there 🤔
Thanks for calling that out @madhusudhand, I've done an update to this PR to account for left margins, so a top/left shadow now looks like this:
To recap:
Thanks again for all the great feedback! 🙇 |
|
Oh right, the iframe. I was about to respond to this one:
... namely that this is also something you could accomplish in your site, just add a column of buttons, so it would seem fair to represent this here as well. I would prefer if we could avoid the border, but I can see how it might be the only real solution to convey what's going on here. But I want to ask a possibly obvious question, forgive me if it isn't a good fit: do we actually have to show every single block inside their own iframes? I know it uses the preview component, where one of its tasks is to protect it and its surroundings from CSS bleed. But could we update this component to make the iframe optional? The stylebook is conceptually similar to your editing canvas, meaning it could just be all the blocks shown, row by row, as they would be shown in the editing canvas, no iframes in sight except perhaps that of the canvas itself? |
That's a great question — I actually don't know the answer to this one. I assume the iframe is usually in use because block previews are displayed in places that aren't the editor canvas, so it probably helps prevent styles bleeding along with allowing styles to constrain the preview (scale, etc). But as you mention, in this case, we actually want the blocks to be properly rendered inline. I'll do a little digging to see if there's a reason it can't be done, it very much sounds like an idea worth investigating to me 👍 |
|
Okay, after a bit of hacking around, I think Joen's idea sounds the most promising so far! It'll involve a bit of refactoring of the Style Book, but I think it'll get us closer to the ideal that the Style Book is really previewing how things should look in the editor, rather than be a number of separate iframes. Here's what I've managed to hack together so far:
In the above screenshot:
That last point means that it'll involve a little wrangling to get the styling to work correctly within the iframe of the Style Book content area. I still need to do a bit more tidying of my experiment locally before pushing to a draft PR, but if there aren't any objections, I'll press on exploring this direction, as I think it'll likely help us for future block supports / styling things beyond big shadows in the future. The main argument I can think of against moving the iframe up to wrap all of the content of the Style Book is if we were planning to add fancier components to the canvas of the Style Book at any point in the future. That seems unlikely to me, and personally I'd lean toward trying to treat the canvas area as closely as possible as if it were kind of an editor canvas. Happy for any feedback on that, of course. Thanks again for the feedback so far, everybody! |
|
Oooh this looks good! Can we perhaps add the necessary subset of (duly scoped) editor styles to the iframe? It's really just block names we need to show, right? |
I think so! It should be pretty do-able, I think. I've put up a hacky draft PR over in #48664 with my work from today, and I managed to copy + paste some styling for the headings / buttons over, and it looks promising so far. I'll continue tweaking it tomorrow and see how stable we can get it 😀 |
ntsekouras
removed
the
Backport to WP 6.7 Beta/RC
What?
Fixes: #48392
This PR updates the styling rules for the Style Book so that there's a little more breathing room within the preview to account for the most common box shadow values. This helps prevents the Style Book from cutting off the shadows in the previews.
Note that this is a naive approach that just adds a little spacing. I also hacked away at an attempt to see if we can extract the shadow values to determine the exact amount of margin to add, but this turned out to be quite complex — we would need to both parse the shadow value, and also iterate over all blocks within the preview to see if it contains any blocks that have a shadow. Otherwise, Button within a Buttons block preview will not trigger additional margins to be added. I'm happy to continue exploring this in a follow-up if folks think the "proper" approach is worth it.
Why?
As reported in #48392 box shadow values fall outside of the content area of the block preview within the Style Book and were being cut off. This PR seeks to remedy this.
How?
16pxtop and bottom margin to the preview in the Style Book16pxtop padding to the block title text so that it still lines up with the previewTesting Instructions
Screenshots or screencast