TreeSelect: remove margin overrides and add new opt-in prop #47515
Conversation
brookewp
added
[Type] Enhancement
packages/editor/src/components/post-taxonomies/hierarchical-term-selector.js
Show resolved
Hide resolved
|
Size Change: -4 B (0%) Total Size: 1.34 MB
ℹ️ View Unchanged
|
brookewp
force-pushed
the
update/treeselect-margin
branch
from
9022f41
to
02b8e80
Compare
|
Flaky tests detected in 73f37cc. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4561486053
|
packages/editor/src/components/post-taxonomies/hierarchical-term-selector.js
Show resolved
Hide resolved
packages/editor/src/components/post-taxonomies/hierarchical-term-selector.js
Show resolved
Hide resolved
brookewp
force-pushed
the
update/treeselect-margin
branch
from
02b8e80
to
12e2cde
Compare
| // Adjust margin-bottom from being applied to the `BaseControl` | ||
| .wp-block-latest-posts__sorting-filtering .components-base-control { | ||
| margin-bottom: $grid-unit-10; | ||
| } |
There was a problem hiding this comment.
To clarify, what we want is to "un-apply" or in other words "opt out" the QueryControl components from these overrides:
That means to apply margin-bottom 0 (or auto), which is the neutral state. Similar to what we're doing for the FocalPickerControl just above. This "blank slate" allows us to use clean flex gap styling within QueryControl itself. The CSS scope is .components-base-control in all QueryControls, not just in the Latest Posts block.
I may have misled you when I said we probably didn't have to add new CSS classes. I was misreading these keys as class names 😆
This may be conceptually confusing, so feel free to ping me if you'd like to discuss further!
There was a problem hiding this comment.
That means to apply margin-bottom 0 (or auto), which is the neutral state. Similar to what we're doing for the FocalPickerControl just above. This "blank slate" allows us to use clean flex gap styling within QueryControl itself.
Thanks for clarifying, @mirka! This is clear to me.
The CSS scope is .components-base-control in all QueryControls, not just in the Latest Posts block.
And this makes sense, but I have a few questions. 😄
In this PR, QueryControls appears to match trunk in Storybook when adding VStack with a gap of 8px. But if we remove the BasecControl inspector override as we did with FocalPickerControl, the spacing looks off in the Latest Posts block (screenshot below), due to other panels in the inspector that still have those bottom margins. So would we then need to add a specific CSS override to the Latest Posts block after to fix the issue this would cause?
I believe the Latest Posts block is the only usage of QueryControls in GB. Isn't the BaseControl override only an issue within the inspector within GB? Or are there other areas that could be affected?
There was a problem hiding this comment.
You make a good point — it seems like we have two choices:
- Go ahead with the solution suggested by @mirka — good in principle, but it may introduce some visual inconsistencies when looking at the Latest Posts controls
- Leave the
BaseControloverrides as they are currently.
We could opt for 2, and remove the BaseControl margins separately in the future?
Finally, I just noticed that QueryControls doesn't seem to have a root classname that we could use anyway (i.e. components-query-control).
There was a problem hiding this comment.
You're asking whether it's ok to have 8px margins for QueryControl controls while the rest of the Latest Posts controls have 24px margins, correct?
Our new controls grid is 16px gap (not 8px), so just using 16px for the VStack already will ease the awkwardness a bit.
I don't have a strong opinion on what the exact margin values should be in the Latest Posts block inspector, but I think the minimal requirements we should hit in this PR are:
- It looks correct in the Storybook (16px gap).
- Any QueryControl component injected into the block inspector sidebar should look correct (not just in Latest Posts, because e.g. third-party blocks could use it).
What do you think?
There was a problem hiding this comment.
_ tl;dr, if we have it look correct in storybook, it won't look good for the Latest Posts block, which appears to be the only use case of QueryControls in GB._
Aside from what I've done in this PR and #48609 to fix the spacing below the help text, I'm not sure what the next step is. 😕
You're asking whether it's ok to have 8px margins for QueryControl controls while the rest of the Latest Posts controls have 24px margins, correct?
@mirka, I'm not sure if this was directed at me or at @ciampo, but I'll share my thoughts in case I wasn't clear.
I had set the spacing to 8px, to match what QueryControls looks like in trunk. If it makes sense to update to the new controls grid spacing, then I'm happy to do that (well I already did in my most recent commit 😂 )
My original concern was the result on the Latest Posts block, with either the 8px or 16px spacing because with the override, the space becomes massive:
It looks correct in the Storybook (16px gap).
Any QueryControl component injected into the block inspector sidebar should look correct (not just in Latest Posts, because e.g. third-party blocks could use it).
Your minimum requirements make perfect sense. In regards to third-parties, would the block inspector override affect them or is it contained to GB?
I thought it was the latter, which is why I thought we could add the override in this case/in the meantime, while we can't remove the margin from the inspector.
There was a problem hiding this comment.
What I had in mind is pretty simple — to adopt the same "opt out" pattern we established in FocalPointPicker. Something like this 👉 6cd387e
In regards to third-parties, would the block inspector override affect them or is it contained to GB?
The former. The block inspector override will affect third-party usages of QueryControls as long as they are rendered in the block inspector. This is why we can't just patch it for Latest Posts, it has to work for all QueryControls.
There was a problem hiding this comment.
Thanks for the context and the commit! When reviewing the commit, I see the option to pull the request. Is that the next step here? (also need to rebase due to changelog). I haven't seen this before, so just wanted to check!
There was a problem hiding this comment.
Good question! You can do whatever you want with it, including pulling or cherry-picking or just taking inspiration from the diff 😄 Another fun tip is to append .patch to the commit URL and you'll get a patch file.
There was a problem hiding this comment.
Another fun tip is to append .patch to the commit URL and you'll get a patch file.
🤯
| // Adjust margin-bottom from being applied to the `BaseControl` | ||
| .wp-block-latest-posts__sorting-filtering .components-base-control { | ||
| margin-bottom: $grid-unit-10; | ||
| } |
There was a problem hiding this comment.
You make a good point — it seems like we have two choices:
- Go ahead with the solution suggested by @mirka — good in principle, but it may introduce some visual inconsistencies when looking at the Latest Posts controls
- Leave the
BaseControloverrides as they are currently.
We could opt for 2, and remove the BaseControl margins separately in the future?
Finally, I just noticed that QueryControls doesn't seem to have a root classname that we could use anyway (i.e. components-query-control).
brookewp
force-pushed
the
update/treeselect-margin
branch
from
12e2cde
to
5342eb6
Compare
brookewp
force-pushed
the
update/treeselect-margin
branch
from
3185afa
to
4fa9a02
Compare
# Conflicts: # packages/components/CHANGELOG.md
What?
Added new opt-in prop
__nextHasNoMarginBottomfor usages ofTreeControlin the Gutenberg codebase and removed margin overrides.Why?
Part of this project: #38730
The tl;dr is
BaseControlhas amargin-bottomwhich makes it difficult to reuse and results in inconsistent use.How?
By removing margin overrides in the CSS and adding the prop
__nextHasNoMarginBottom.Testing Instructions
HierarchicalTermSelector
CategorySelect & AuthorSelect
I don't believe that
CategorySelectshows up anywhere/matches any conditions based on this part of the readme:So to test it, the only way I found was to replace
categorySuggestions={ categorySuggestions }withcategoriesList={ categories }on line 343 inpackages/block-library/src/latest-posts/edit.js:gutenberg/packages/block-library/src/latest-posts/edit.js
Line 343 in 6a64cdf
Then: