TextControl: remove margin overrides and add new opt-in prop (pt 1/2) #47157
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
|
Size Change: -264 B (0%) Total Size: 1.33 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in 8076070. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/3917093720
|
brookewp
force-pushed
the
update/textcontrol-margin
branch
from
fb38e7d to
d583e11
Compare
brookewp
commented
Jan 14, 2023
| @@ -247,6 +247,7 @@ export default function QueryInspectorControls( { | |||
| onDeselect={ () => setQuerySearch( '' ) } | |||
| > | |||
| <TextControl | |||
| __nextHasNoMarginBottom | |||
There was a problem hiding this comment.
To note, there is an additional margin override, but it appears to be coming from ToolsPanel:
brookewp
commented
Jan 14, 2023
| expanded={ false } | ||
| > | ||
| <FlexItem> | ||
| <VStack spacing={ 3 }> |
There was a problem hiding this comment.
This is one of many, but we could have continued to Flex and Flex Item but it requires more. Also, before, the first Flex/parent> Flex Item/child wasn't affecting the styles. So I thought it would be cleaner to nest an HStack within a VStack for the same effect but with less.
And it looks like it's been discussed before, to move from Flex to VStack/HStack: #43085
brookewp
commented
Jan 14, 2023
| @@ -49,10 +49,7 @@ | |||
| .edit-post-template-top-area__popover { | |||
| .components-popover__content { | |||
| min-width: 280px; | |||
|
|
|||
| > div { | |||
| padding: 0; | |||
There was a problem hiding this comment.
This wasn't affecting the popover so it appeared to be broken (shared screenshots in testing steps).
There was a problem hiding this comment.
Nice catch!
Unrelated to this PR, but just to note some clean ways to resolve these common .components-popover__content hacks in Dropdown:
padding: 0— We recently introduced a<DropdownContentWrapper>that can be used to remove the default padding.min-width— This style can easily be moved to the element inrenderContent, thus avoiding a CSS hack.
There was a problem hiding this comment.
Thank you for sharing!
brookewp
changed the title
mirka
approved these changes
Jan 16, 2023
| justify="flex-end" | ||
| > | ||
| <FlexItem> | ||
| <VStack spacing="5"> |
| ) } | ||
| /> | ||
| <HStack | ||
| className="edit-site-custom-generic-template__modal-actions" |
There was a problem hiding this comment.
Would it be better to remove the margin-top on .edit-site-custom-generic-template__modal-actions and merge that extra whitespace into the VStack spacing?
There was a problem hiding this comment.
Yes, definitely! Nice catch. :)
| @@ -49,10 +49,7 @@ | |||
| .edit-post-template-top-area__popover { | |||
| .components-popover__content { | |||
| min-width: 280px; | |||
|
|
|||
| > div { | |||
| padding: 0; | |||
There was a problem hiding this comment.
Nice catch!
Unrelated to this PR, but just to note some clean ways to resolve these common .components-popover__content hacks in Dropdown:
padding: 0— We recently introduced a<DropdownContentWrapper>that can be used to remove the default padding.min-width— This style can easily be moved to the element inrenderContent, thus avoiding a CSS hack.
brookewp
force-pushed
the
update/textcontrol-margin
branch
from
8076070 to
62fe52c
Compare
49 tasks
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.
Part 1/2
What?
Added new opt-in prop
__nextHasNoMarginBottomfor usages ofTextControlin 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. As well as addingVStackandHStack(and sometimes replacingFlexwith those) to remove additional CSS and get the changes visually identical to trunk.Testing Instructions
Block Editor testing steps:
Create a new post and follow testing steps below:
PostSlug
add_post_type_support( 'post', 'slug' );togutenberg.phpPostURL
! isEditableto view it: https://github.com/WordPress/gutenberg/blob/fb38e7dbe99669cfc0c569903b34809974f68fc3/packages/editor/src/components/post-url/index.js#L52Link Control
ButtonEdit & withInspectorControl (HTML/CSS fields)
Image
ReusableBlockEdit & ReusableBlockConvertButton
Step 1
Step 3
OrderedListSettings
NavigationLinkEdit & NavigationSubmenuEdit & NavigationMenuNameControl
Step 3
Step 6 and 8
PostFeaturedImageDisplay
PostTermsEdit
QueryInspectorControls
TableEdit
PostTemplateCreateModal & EditTemplateTitle
Before
After
PostPublishPanelPostpublish
Block Editor checklist:
Site Editor testing steps:
AddCustomGenericTemplateModal & RenameMenuItem
Step 5
Step 8
PostTitleEdit
TemplatePartAdvancedControls
Site Editor checklist: