TextControl: remove margin overrides and add new opt-in prop (pt 2/2) #47158
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: -584 B (0%) Total Size: 1.33 MB
ℹ️ View Unchanged
|
brookewp
commented
Jan 14, 2023
| justify="flex-end" | ||
| > | ||
| <FlexItem> | ||
| <VStack spacing="5"> |
There was a problem hiding this comment.
Added VStack with spacing to be consistent with other modals, as the spacing between the input and button wasn't the same as other modals in the site editor.
brookewp
commented
Jan 14, 2023
| className="edit-site-create-template-part-modal__area-base-control" | ||
| > | ||
| <RadioGroup | ||
| <VStack spacing="4"> |
There was a problem hiding this comment.
I personally think that the new spacing looks more centered, but I could be wrong. I can add CSS to make it like trunk if others prefer.
brookewp
commented
Jan 14, 2023
|
|
||
| .edit-site-create-template-part-modal__area-base-control .components-base-control__label { | ||
| margin: $grid-unit-20 0 $grid-unit-10; | ||
| cursor: auto; |
There was a problem hiding this comment.
Is there a reason why we'd use a text cursor here?
There was a problem hiding this comment.
Wow this is interesting, I've never noticed it before. The standard WordPress forms.css adds a cursor: pointer to all labels:
I'm guessing the intent was to add an affordance that the labels are clickable. But unfortunately that is not always true, as is in this case (and some others, e.g. <progress>). So that's probably why they explicitly overrode the cursor: pointer here.
cc @ciampo FYI
brookewp
commented
Jan 14, 2023
| @@ -1,18 +1,16 @@ | |||
| .block-editor-date-format-picker { | |||
There was a problem hiding this comment.
We can add CSS to make it the same as trunk, but there is a 20px space so we'd either have to use 16px, 24px, or possibly a grid unit subtracted by another in order to get 20px if we're okay to not use a single grid unit.
brookewp
commented
Jan 14, 2023
| display: flex; | ||
| place-content: space-between; | ||
| margin-top: $grid-unit-40; | ||
| margin-top: $grid-unit-20; |
There was a problem hiding this comment.
Curious about when we should use a Spacer component instead of a margin like in this instance, but it seems like it could be debatable. :)
There was a problem hiding this comment.
Personally I haven't really encountered a legitimate use case for Spacer!
In this case I want to make sure we aren't mixing margin and gap in a haphazard way:
I think it's fine to space the footer row using either margin or gap, but not both 😄
There was a problem hiding this comment.
Far enough! The reasoning here was to get closer to matching trunk. If the narrow gap is okay, I'd rather remove the margin altogether. Or add Spacer unless that's also weird to have a gap and Spacer. But then I know you'd like to avoid Spacer in general! 😅
There was a problem hiding this comment.
Sorry I wasn't clear, we should maintain the same amount (32px) of whitespace. The point is to achieve it "logically", with a separate VStack, or a single margin-top:
There was a problem hiding this comment.
Thanks for clarifying! I missed this comment earlier but my latest commit covers this now.
brookewp
commented
Jan 14, 2023
| @@ -44,6 +44,7 @@ const Template: ComponentStory< typeof Placeholder > = ( args ) => { | |||
| <Placeholder { ...args }> | |||
| <div> | |||
| <TextControl | |||
| __nextHasNoMarginBottom | |||
There was a problem hiding this comment.
Adding to stories, as we did with Disabled in a related PR. The spacing is also similar to what we did with the legacy widget in the SelectControl PR,
|
Flaky tests detected in ac7ba5d. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4002185476
|
brookewp
changed the title
mirka
reviewed
Jan 17, 2023
Comment on lines
-10
to
-14
| .block-editor-date-format-picker__custom-format-select-control { | ||
| &.components-base-control { | ||
| margin-bottom: 0; | ||
| } | ||
| } |
There was a problem hiding this comment.
It looks the BaseControl here is used merely for its implicit 8px margin-bottom properties (which is exactly the thing we're deprecating 😄). I think we can remove the BaseControl altogether and just wrap the CustomSelectControl and TextControl in a VStack.
There was a problem hiding this comment.
Great find! I'll keep an eye out for stuff like this in the future
Comment on lines
112
to
114
| .block-library-video-tracks-editor__single-track-editor-track-title, | ||
| .block-library-video-tracks-editor__single-track-editor-source-language { | ||
| flex: 1; |
There was a problem hiding this comment.
This looks like a use case for Grid rather than HStack, since we know we want two equal-width columns.
There was a problem hiding this comment.
Awesome! I should have known there was a Grid component out there :) Thanks!
| display: flex; | ||
| place-content: space-between; | ||
| margin-top: $grid-unit-40; | ||
| margin-top: $grid-unit-20; |
There was a problem hiding this comment.
Personally I haven't really encountered a legitimate use case for Spacer!
In this case I want to make sure we aren't mixing margin and gap in a haphazard way:
I think it's fine to space the footer row using either margin or gap, but not both 😄
| <div className="block-library-video-tracks-editor__single-track-editor"> | ||
| <VStack | ||
| className="block-library-video-tracks-editor__single-track-editor" | ||
| spacing="3" |
There was a problem hiding this comment.
One thing I noticed is that spacing=3 makes it a little hard to tell that the help text is associated with the items on top. It does seem like an insufficient amount of whitespace when help text is involved. I'll leave it up to you, but this might be a good time to switch this to the new grid spacing guidance, which would be spacing=4.
There was a problem hiding this comment.
I agree! And nice, I love consistent spacing; sooo glad to see there are guidelines! 😍
|
|
||
| .edit-site-create-template-part-modal__area-base-control .components-base-control__label { | ||
| margin: $grid-unit-20 0 $grid-unit-10; | ||
| cursor: auto; |
There was a problem hiding this comment.
Wow this is interesting, I've never noticed it before. The standard WordPress forms.css adds a cursor: pointer to all labels:
I'm guessing the intent was to add an affordance that the labels are clickable. But unfortunately that is not always true, as is in this case (and some others, e.g. <progress>). So that's probably why they explicitly overrode the cursor: pointer here.
cc @ciampo FYI
| className="edit-site-create-template-part-modal__area-base-control" | ||
| > | ||
| <RadioGroup | ||
| <VStack spacing="4"> |
brookewp
force-pushed
the
update/textcontrol-margin-with-small-changes
branch
from
07cd8e6 to
74e2a4a
Compare
49 tasks
|
@mirka, just to note, I also included the file I originally intended to include it in this PR due to some minor visual changes, but I ended up with it matching trunk. I just remembered that I needed to move it to part 1, but I merged that already. However, in the spirit of part 2, it would be possible to remove most of the CSS (this has |
…inor visual changes
brookewp
force-pushed
the
update/textcontrol-margin-with-small-changes
branch
from
749debf to
ac7ba5d
Compare
|
Always good to see more 🔴 than 🟢 in the diff! Thank you @brookewp for working on this! |
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 2/2
What?
Added new opt-in prop
__nextHasNoMarginBottomfor usages ofTextControlin the Gutenberg codebase and removed margin overrides.This is part of #47157 but was separated due to small visual changes to each of the blocks/components in this PR.
Why?
These changes will result in less CSS used in favour of components.
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.Additional Notes
packages/block-library/src/post-comment/edit.jshas been deprecated. I looked at it in the editor, and it looks a bit broken as it is:Adding the prop won't affect it by much, it shifts slightly down a pixel (similar to the legacy widget in the SelectControl PR, which we updated without adding additional overrides), so I added it here to be consistent.
Testing Instructions
Rather than verifying if the changes look the same as trunk, we'll need to make sure that these minor visual changes are okay or if we need to add CSS overrides to match perfectly to trunk.
Block Editor testing steps:
HierarchicalTermSelector
gutenberg/packages/editor/src/components/post-taxonomies/hierarchical-term-selector.js
Line 40 in f7052d3
NonDefaultControls (DateFormatPicker)
Before
After
ImageURLInputUI
Before
After
SingleTrackEditor
Before
After
SocialLinkEdit
Before
After
Site Editor testing steps:
CreateTemplatePartModal
Before
After
TitleModal
Before
After
Storybook testing steps:
Placeholder
npm run storybook:devBefore
After