Extract a DimensionsPanel component as a reusable component between Global Styles and Block Inspector #48070
Conversation
youknowriad
added
[Type] Code Quality
youknowriad
requested review from
oandregal,
noisysocks,
glendaviesnz,
jorgefilipecosta,
andrewserong and
aaronrobertshaw
|
Size Change: -192 B (0%) Total Size: 1.33 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in e198c8a. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4260642543
|
packages/block-editor/src/components/global-styles/dimensions-panel.js
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Thanks for the hard work on this @youknowriad. It's great to see so much of the boilerplate code being removed here. 👍
So far, with the noted exception of the spacing visualizers, it is testing pretty well. The only issue that came up in a quick smoke test was that the spacing presets control isn't shown when editing individual blocks as it does on trunk.
| Before | After |
|---|---|
 |
 |
I'm happy to keep reviewing as this PR evolves though I'll be AFK until later next week.
youknowriad
force-pushed
the
extract/dimensions-panel
branch
3 times, most recently
from
670fcd0
to
4a4c78c
Compare
youknowriad
force-pushed
the
extract/dimensions-panel
branch
from
4a4c78c
to
49966b8
Compare
youknowriad
force-pushed
the
extract/dimensions-panel
branch
from
cd20917
to
6b86fab
Compare
|
I think this PR should be ready for final testing :) |
youknowriad
force-pushed
the
extract/dimensions-panel
branch
from
4d42e70
to
b073036
Compare
There was a problem hiding this comment.
This is looking pretty close to me (and so good to see how this results in ~750 lines being removed). I ran into a couple of bugs in testing:
Global styles
❌ contentSize (doesn't update in the editor canvas and doesn't appear to save)
❌ wideSize (doesn't update in the editor canvas and doesn't appear to save)
✅ padding
✅ margin
✅ block spacing
✅ minimum height
Individual block
✅ padding
❌ margin (resetting individual value causes a block error)
✅ block spacing
✅ minimum height
✅ child layout controls (fit, fill, width)
✅ injected controls like min height of cover block, aspect ratio in post featured image
✅ visualizers for padding and margin
Screengrabs
| contentSize / wideSize not updating / saving | resetting individual margin control on Group block causes block error |
|---|---|
 |
 |
packages/block-editor/src/components/global-styles/dimensions-panel.js
Outdated
Show resolved
Hide resolved
youknowriad
force-pushed
the
extract/dimensions-panel
branch
from
b073036
to
e46b52e
Compare
|
Thanks for the great testing @andrewserong I've fixed these issues. I'm not 100% satisfied with the fix for the layout properties in Global styles but I think it's a symptom of the fact that "layout" is actually stored in "settings" rather than "styles" in the global styles config. I'm not changing that today :) Also it could be improved a bit if we have a separate "LayoutPanel" like we do in the block inspector. Actually, in terms of UX we should try to unify a bit later, in inspector layout is separate from the dimensions panel but it's included there in global styles. |
youknowriad
force-pushed
the
extract/dimensions-panel
branch
from
e46b52e
to
e198c8a
Compare
There was a problem hiding this comment.
Thanks again for all the follow-up, here, it's looking very close to me. All the previously flagged issues have been resolved, the one issue I encountered in re-testing is that the child layout Width controls don't appear to be able to hidden once revealed, with this PR applied. When I go to reset the Width control after adding it, it resets the value, but doesn't hide it.
| Trunk | This PR |
|---|---|
 |
 |
Oh, actually, I see from inspecting the block markup, it looks like an SVG element is accidentally being injected into the style.layout attributes when it's being reset:
Looks like a one-liner to fix it up, I've left a comment 🙂
packages/block-editor/src/components/global-styles/dimensions-panel.js
Outdated
Show resolved
Hide resolved
|
Thanks for the deep testing @andrewserong it should be fixed now |
There was a problem hiding this comment.
This is testing really well for me now, I haven't managed to break it again 😄
Thanks again for all the follow-up here, I think this is in good shape to land now, and I think the trade-offs made (e.g. layout controls in dimensions) make good sense, and we'll also be able to keep tweaking code quality things in follow-ups. One of the things I liked about the shape of the DimensionsPanel code in the site editor, is that we'd managed to further extract some of the inline definitions of things like setPaddingValue into hooks like usePaddingProps, so it might be worth pursuing that sort of approach again in the future, if we find that the DimensionsPanel becomes hard to read in its current form. But that's not at all a blocker, and just an idle thought from reading through the code again.
This LGTM now, so giving it the ✅ — do feel free to wait for another approving review if you're concerned about whether we've covered everything, but I'm feeling pretty confident about this change now after the last few rounds of testing and poking it 😄
Thanks again for all the continued work consolidating these components! ✨
FYI, I'm currently halfway through testing and reviewing this PR as well. There are a couple of small things we should probably tweak code-wise before merging. I'll complete the review shortly. |
There was a problem hiding this comment.
Appreciate the hard work iterating on this one 🙇
As Andrew noted, this is now testing really well.
Global styles
✅ Padding & margin
✅ Block gap
✅ Min height
✅ ContentSize & wideSize
✅ Typography panel
Individual blocks
✅ Padding & margin
✅ Block gap
✅ Visualizers
✅ Min height
✅ Child layouts
✅ Non-block-support controls e.g. aspect ratio in Post Feature Image
✅ Typography panel
I've left a few inline comments for minor things we might be able to clean up.
While reviewing the files in this PR I also noticed a typo bug (typography.fuild) introduced in #47356. I'm happy to throw up a quick fix or approve it as part of this PR if you'd prefer.
packages/block-editor/src/components/child-layout-control/index.js
Outdated
Show resolved
Hide resolved
This sounds like a good follow-up when bandwidth permits. I had similar thoughts reading through that section of the code. |
I saw this pattern :), I wondered whether I should apply it here, I'm hesitant and also not totally convinced. I was wondering if sub components is better here than several hooks. That said I consider this as "implementation details" of the component itself and since it has no impact on the API, so I don't really care too much. |
|
I think I've addressed the suggestion here. Thanks all for the reviews, I really appreciate it. Let's see if we can continue with other panels. |
|
Nice work landing this one!
I agree, it's definitely just the "implementation details" side of things, and sub-components would be a good thing to explore and neater than a proliferation of hooks 👍. From my perspective, as these components get bigger, it'd be good to try to tidy them up a bit where we can to improve readability, but definitely not a blocker for anything 🙂 |
|
I put up a fix for the typo breaking the fluid typography setting in #48605. |
|
This PR introduced a small regression to the resetting of dimensions controls that have only axial values (horizontal & vertical) e.g. the Button block. With this PR, the A fix is up in #50654 |
Related to #37064 and #47348 and similar to #47356 (and follow-ups)
What?
A year ago now, I've opened a discussion in #37064 that proposes unifying the components that are used in the block inspector and global styles. The idea is that we'd have components that look like this:
That component would render a panel used to edit a given style (say "typography" or "colors"), the
valuehas a well specified prop (that is mapped over the "style" object of a block or a global style "node" ). The idea is that we could use that exact same panel in both places, the only difference is a wrapper that map the "value" to the right place:Recently we realized this vision for the TypographyPanel and the current PR does the exact same thing for the DimensionsPanel and uses the new UI component for both inspector controls and global styles.
To do
Testing Instructions