ToolsPanel: Use Grid component to handle layout #35621
Conversation
aaronrobertshaw
added
the
[Feature] UI Components
aaronrobertshaw
requested review from
ajitbohra and
chrisvanpatten
as code owners
|
Size Change: -3 B (0%) Total Size: 1.07 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
Nice work, @aaronrobertshaw, this looks great in the Storybook demo, I think this is going to allow for some really nice layouts. It's quite fun to interact with:
I like the addition of being able to set columns at the ToolsPanel level. While I imagine we won't be refactoring away from Grid, I was wondering if using gridColumn at the ToolsPanelItem level ties the prop closely to the specific implementation? I could imagine a plugin author going to add a ToolsPanelItem to an existing panel and not being sure what value to put into the gridColumn string. Would an integer based columnSpan prop be clearer as an API? Also, if we ever needed to refactor to flex, the integer value could be used to calculate flex-basis without the ToolsPanelItem needing to change its value.
Of course, on the other hand, the gridColumn prop passing the value directly to the CSS does give the ToolsPanelItem more control if someone wanted to set a different grid-column-start / grid-column-end value than just spanning, so keeping it as is could offer more flexibility.
Curious to hear what you and other folks think, but this is testing nicely for me in Storybook and in existing ToolsPanels in the editor 👍
|
Thanks for taking this for a test drive 👍
This is why I went with the current approach. More power and flexibility for free and it aligns more directly with an external spec i.e. CSS.
I mentioned the direct mapping to the CSS property in the README so it should be clear. I originally had a link to external CSS specs but couldn't find many other cases of doing that so removed it.
I did consider it however it seemed to be unnecessarily restrictive when we could get the positioning functionality for free otherwise. If we needed to refactor to flex |
Thanks for explaining the rationale! Agreed, since it offers better flexibility and matches the CSS spec, that sounds like a good way to go. And it avoids adding an extra abstraction with the columnSpan idea that didn’t really map to the right CSS property anyway 👍 |
aaronrobertshaw
force-pushed
the
refactor/toolspanel-to-use-grid
branch
from
8837ffd to
357dbc3
Compare
There was a problem hiding this comment.
I'm wondering what happens in cases like this, where you have an implicit group of optional panel items that belong in their own row. For example width/height/depth and padding/margin. The "wrap" behavior ("auto-placement"? unsure what the correct term is!) is not what you want:
CleanShot.2021-10-16.at.01.46.10.mp4
Setting explicit grid-column-start values will make it better, but then it doesn't auto-place within the row:
CleanShot.2021-10-16.at.02.26.36.mp4
I'm probably missing context on how ToolsPanel is intended to be used, but I was actually imagining the consumer being responsible for doing the layout (Grid or otherwise) inside ToolsPanel.
<ToolsPanel>
<Grid columns={2} gap={2}>
<ToolsPanelItem />
<ToolsPanelItem />
</Grid>
<Grid columns={3} gap={2}>
<ToolsPanelItem />
<ToolsPanelItem />
<ToolsPanelItem />
</Grid>
</ToolsPanel>One could also say that an implicit group (like width/height/depth) should be encapsulated inside a single ToolsPanelItem. In that case, it might be that an elaborate gridColumn system built into ToolsPanel is a bit overkill, because there is a limit to the amount of random auto-placement you want to occur on unrelated optional items that are not full-width.
Just a non-blocking observation, please disregard if it doesn't matter 🙂
|
Thanks for the feedback @mirka 👍 I'll explore the suggestion further and see what I can come up with.
I believe the idea was for all the controls within a Perhaps @jasmussen might have some insight as to what the
At a glance, this would provide more flexibility though I'm not sure how it would play when SlotFills are used to allow injection of controls into the panel. We'd likely be back to adding a generic grid wrapping all those fills and this same limitation/issue. This is something I'll need to dig into a little deeper.
I'm not sure I agree with this. These individual items need to be represented within, display controlled, or value reset, by the tools panel menu. The current approach in the If it turns out to be more involved, given this was non-blocking as you mentioned, a follow-up PR refining this layout could be an option. |
|
The neat arrangement of controls in the inspector, as more are added or removed, is definitely a tricky one, and important one to get right. In many ways, that's a key part of addressing #27331: ensuring a system where controls that get shown or hidden as part of theme.json or global styles or users, stil arranges itself in a neat way, and can appear consistent across inspectors. Ultimately I'm not personally sure how far we can go with regards to controls resizing and adjusting themselves depending on context, I'll need to defer to you all (and CC: @ciampo as well). But just as a baseline, I imagine controls registering themselves as half or full-size controls, possibly with an option for 3rds as well, and then as one half-size item gets hidden, it will make room for a subsequent half-size item to take its place: It doesn't seem like it will be necessary for a full size control to be able to shift between full-size and half-size automatically, but it's an optimization that could be looked at if/when it becomes necessary. |
|
Thanks for sharing that @jasmussen 👍
This sounds to me like the current behaviour. The I think the question Lena is asking is whether that next subsequent half-size item should in fact take its place in some situations.
After a little testing, I believe we can achieve the "implicit group layout" raised earlier with only a small tweak to the styles to hide an empty grid. The items in the group would be wrapped in a Screen.Recording.2021-10-18.at.5.48.43.pm.mp4Here's a diff for the style tweak and storybook example changes demo'd in the above video.diff --git a/packages/components/src/tools-panel/stories/index.js b/packages/components/src/tools-panel/stories/index.js
index e9a39b2932..e5b7d9b73e 100644
--- a/packages/components/src/tools-panel/stories/index.js
+++ b/packages/components/src/tools-panel/stories/index.js
@@ -15,6 +15,7 @@ import { ToolsPanel, ToolsPanelItem } from '../';
import Panel from '../../panel';
import UnitControl from '../../unit-control';
import { createSlotFill, Provider as SlotFillProvider } from '../../slot-fill';
+import { Grid } from '../../grid';
export default {
title: 'Components (Experimental)/ToolsPanel',
@@ -258,45 +259,49 @@ export const WithCustomColumns = () => {
onChange={ ( next ) => setSize( next ) }
/>
</ToolsPanelItem>
- <ToolsPanelItem
- gridColumn="span 1"
- hasValue={ () => !! width }
- label="Width"
- onDeselect={ () => setWidth( undefined ) }
- isShownByDefault={ true }
+ <Grid
+ style={ { gridColumn: '1/-1' } }
+ columns={ 3 }
+ columnGap={ 16 }
+ rowGap={ 24 }
>
- <UnitControl
+ <ToolsPanelItem
+ gridColumn="span 1"
+ hasValue={ () => !! width }
label="Width"
- value={ width }
- onChange={ ( next ) => setWidth( next ) }
- />
- </ToolsPanelItem>
- <ToolsPanelItem
- gridColumn="span 1"
- hasValue={ () => !! height }
- label="Height"
- onDeselect={ () => setHeight( undefined ) }
- isShownByDefault={ true }
- >
- <UnitControl
+ onDeselect={ () => setWidth( undefined ) }
+ >
+ <UnitControl
+ label="Width"
+ value={ width }
+ onChange={ ( next ) => setWidth( next ) }
+ />
+ </ToolsPanelItem>
+ <ToolsPanelItem
+ gridColumn="span 1"
+ hasValue={ () => !! height }
label="Height"
- value={ height }
- onChange={ ( next ) => setHeight( next ) }
- />
- </ToolsPanelItem>
- <ToolsPanelItem
- gridColumn="span 1"
- hasValue={ () => !! depth }
- label="Depth"
- onDeselect={ () => setDepth( undefined ) }
- isShownByDefault={ true }
- >
- <UnitControl
+ onDeselect={ () => setHeight( undefined ) }
+ >
+ <UnitControl
+ label="Height"
+ value={ height }
+ onChange={ ( next ) => setHeight( next ) }
+ />
+ </ToolsPanelItem>
+ <ToolsPanelItem
+ gridColumn="span 1"
+ hasValue={ () => !! depth }
label="Depth"
- value={ depth }
- onChange={ ( next ) => setDepth( next ) }
- />
- </ToolsPanelItem>
+ onDeselect={ () => setDepth( undefined ) }
+ >
+ <UnitControl
+ label="Depth"
+ value={ depth }
+ onChange={ ( next ) => setDepth( next ) }
+ />
+ </ToolsPanelItem>
+ </Grid>
<ToolsPanelItem
gridColumn="span 2"
hasValue={ () => !! padding }
diff --git a/packages/components/src/tools-panel/styles.ts b/packages/components/src/tools-panel/styles.ts
index d9aa409abd..405a621510 100644
--- a/packages/components/src/tools-panel/styles.ts
+++ b/packages/components/src/tools-panel/styles.ts
@@ -31,6 +31,10 @@ export const ToolsPanel = css`
border-top: ${ CONFIG.borderWidth } solid ${ COLORS.gray[ 200 ] };
margin-top: -1px;
padding: ${ space( 4 ) };
+
+ > div:empty {
+ display: none;
+ }
`;
/**If this satisfies both giving consumers the ability to group controls while still allowing a default layout to be applied by the I'm open to other ideas or thoughts in case I've missed something. |
Yes, I'm missing some nuance. For example, I'm not sure we should introduce thirds yet — it's possible it might be necessary in the future, but it doesn't seem like something to optimize for, especially yet. If we can accomplish the designs in #34574, that feels like it will put us in a good place. But more specifically, I don't know that we'll need to optimize for a design like this:
Especially for the tools panel, we'll curate the sequence of panels to always be in the same sequence. For example, full-width controls will almost always be at the top, with half size controls below that. So you shouldn't encounter a situation where you remove a half-size item, and a full size item can't take it's place. So to rephrase it a bit: the panels need smarts so that when items are hidden or added, controls reorient themselves. However we'll nearly always want to curate the sequence of items. |
|
Sorry for being a bit late to the conversation! I haven't really got the the time to review the code patches in depth, and from tomorrow I won't be able to provide feedback on GitHub reliably until November 8. Having said that, I believe that a good compromise and a safer approach would be to keep things simple for now — we can always add more complex layout behaviours later. In contrast, adding stuff now that we may need to remove later on will be more disruptive. The approach that I propose for the changes in this PR is:
This solution buys us some time to discuss around a few questions that come to mind, which may need further time and exploration before reaching an agreement:
What do folks think? (Also note, as said earlier, that I may not be able to follow up to this comment in the next days, until November 8) |
@aaronrobertshaw Nice, this addresses my concern perfectly, and it looks hopeful that we can have the flexibility while providing a default layout! That said, Marco's plan of not exposing new props in this PR is probably wise. It warrants some more exploration/discussion, and I'm still doubtful that we need to expose unrealistic (>2 columns) or "implementation detail" ( |
This PR has been updated to address the majority of @ciampo's proposal. The main difference is that items still default to spanning the full width. This is by far the most common use case for the Action Taken
SummaryAfter making the above changes:
I do think all this begs the question though as to whether this PR should be put on ice until the required discussions are had and a decision on behaviour made. As it is now, we're switching to the grid component to handle layout but not really allowing it to do any of that. Given this makes very little in the way of user-facing or consumer-orientated changes, would it be better to leave the status quo so this is iterated on once properly? |
|
Having had some time to reflect, I think it would be best to land this so that the reliance on the The recent switch of the Typography block supports panel to use the |
aaronrobertshaw
force-pushed
the
refactor/toolspanel-to-use-grid
branch
from
c8dbddd to
0d7a4a1
Compare
There was a problem hiding this comment.
Having had some time to reflect, I think it would be best to land this so that the reliance on the
ToolsPanelproviding thesingle-columnclass is removed.
Sounds good, and thanks for the solid improvements and exploration here 👍
Let's also add a brief entry under the "Experimental" section in the changelog.
| .single-column { | ||
| grid-column: span 1; | ||
| } |
There was a problem hiding this comment.
Not super important since our columns API is still in flux, but since story code serve as examples it might be nicer to have a styled( ToolsPanelItem ) rather than use an explicit class name here. (Non-blocking nit)
|
A follow-up to fix the Typography panel layout and letter-spacing control after this PR lands can be found in #35911. It would be best to land these as close together as possible. |
andrewserong
added
the
[Type] Code Quality
There was a problem hiding this comment.
A bit late to the party, but — first of all, kudos to everyone for shipping this PR! It's great to see us moving away from the single-column className.
Regarding the next steps, I think that we may be able to make a better decision after getting a bit more experience in using ToolsPanel in more place sin Gutenberg — what do folks think?
I also left a couple of minor comments, possibly requiring a small follow-up PR.
Related:
Follow-up:
Description
This PR refactors the
ToolsPanelto leverage theGridcomponent to handle its layout internally.By adopting the
Gridcomponent to manage theToolsPanellayout, the panel will be better positioned to expose any custom grid props we desire in future. It will also remove the reliance on the currently hard-codedsingle-columnclass name.How has this been tested?
Manually.
ToolsPanelin the storybook examplesToolsPaneldisplays block support panels and controls correctly.Testing instructions
npm run storybook:devScreenshots
Types of changes
New feature.
The removal of the reliance on the
single-columnclass is a breaking change although the component is still experimental and the only adoption of theToolsPanelthat has merged is the "Dimensions" panel that does not usesingle-column.The Typography panel has now been switched to the ToolsPanel as well however that change hasn't been included in a release yet
Checklist:
*.native.jsfiles for terms that need renaming or removal).