Label components: improve consistency by setting to 8px margin-bottom #37844
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
andrewserong
added
[Feature] UI Components
|
I've added a few folks who've recently edited or worked on label spacing as reviewers — I don't think this is all that high priority, though, (it's mostly a code quality thing) so no worries if you're busy / don't have time to look at this right now! |
andrewserong
added
[Type] Bug
|
Size Change: -13 B (0%) Total Size: 1.13 MB
ℹ️ View Unchanged
|
|
This tested well for me, and the change makes sense to me, definitely seems like this should be |
aaronrobertshaw
approved these changes
Jan 12, 2022
There was a problem hiding this comment.
Thanks @andrewserong for cleaning this up. 👍
I agree that we should be leveraging margins here not padding.
✅ Confirmed consistent margins for all updated components via Storybook
✅ Confirmed consistent margins for updated components via block/site editors
LGTM 🚢
|
Nice! I haven't been able to dive in and test this thoroughly, but I appreciate the attention to detail 👌 |
ciampo
approved these changes
Jan 13, 2022
There was a problem hiding this comment.
Thank you @andrewserong for working on this, it's much appreciated!
I haven't had much time to test the changes individually myself (apart from a quick glance at Storybook), but I the code changes LGTM 🚀
Note that the spacing is slightly different in the labels for the color controls in the background gradient as it sets a different line-height for the labels than other usage. I've constrained this PR to just dealing with bottom padding / margin to avoid the PR becoming too large.
Thanks for the explanation! If I'm understanding correctly, are you thinking of following up with another PR that tackles the line-height discrepancies as well?
ciampo
reviewed
Jan 13, 2022
Comment on lines
-133
to
-134
| margin-bottom: ${ space( 2 ) }; | ||
| padding-bottom: 0; |
There was a problem hiding this comment.
So nice to see these style overrides being cleaned up!
|
Thanks for the reviews, folks!
Yes — I enjoy doing these little code quality improvement PRs for components, but so that I can slot in the work in and around product / feature development I find it a bit easier to try to carve out updating one conceptual thing at a time 🙂. There are a couple of potential conflicts with line-heights to resolve, I think the gradient picker expects a line-height of The other thing I'd like to look at is the setting the browser default left padding on |
andrewserong
deleted the
update/select-control-bottom-padding-for-consistency
branch
That's great to hear, and makes sense @andrewserong!
Yeah, the
Sweet! As always feel free to ping me, I'm always happy to help review these PRs! |
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.
Description
Following on from #37747 and #36387 this PR adjusts the
SelectControl's bottom padding from4pxto a bottom margin of8pxfor consistency with other component labels. The changes for consistency in this PR also include:InputControl's label to be8pxfor consistency with other labels, and switch to usingmargin-bottominstead ofpadding-bottom(since we're dealing with the spacing between the label and another element instead of spacing within the label).BorderRadiusControllabel to use margin-bottom instead of padding-bottom and switch to8pxBoxControlheader to use margin-bottom instead of padding-bottom and switch to8pxCustomGradientPickerstyling to no longer set a bottom padding for the label, as the8pxvalue is now used in the underlyingInputControlcomponentFormTokenFieldcomponent to use a bottom margin of8pxToolsPanelstyles as the underlying label should now have the correct spacing 🤞 (note I've left theline-heightrule in place as there's still some inconsistent usage of that value around the place)A note on padding vs margin: while arguably padding or margin are fairly interchangeable for controlling the spacing between the label and an input element, I think
marginis probably the better fit — if we imagine giving a label a border or a background color, then I thinkmarginbecomes the clearer choice as otherwise the background color or border would sit right next to the input element.How has this been tested?
Tested in Storybook after running
npm run storybook:dev:Tested in real world usage of the above components. Below are some screenshots from the sidebar. Good things to test include:
Note that the spacing is slightly different in the labels for the color controls in the background gradient as it sets a different line-height for the labels than other usage. I've constrained this PR to just dealing with bottom padding / margin to avoid the PR becoming too large.
Screenshots
Types of changes
Bug fix (non-breaking change which fixes an issue)
Checklist:
*.native.jsfiles for terms that need renaming or removal).