Merged
Conversation
Mamaduka
added
[Package] Components
|
Size Change: +18 B (0%) Total Size: 1.13 MB
ℹ️ View Unchanged
|
Contributor
Out of curiosity, could you point out the offending code that introduced the regression in #25842 ? The changes overall LGTM, but I'd appreciate if folks who recently worked on ensuring label alignment also had a look (cc @andrewserong @aaronrobertshaw @mirka ) |
Member
Author
ciampo
reviewed
Jan 7, 2022
Contributor
ciampo
left a comment
ciampo
left a comment
There was a problem hiding this comment.
With #25842, we started using styled-components, and #29550 removed styles files. So I guess regression was caused by both PRs together.
Gotcha! Thank you for the explanation.
I went ahead and left a comment while waiting for other folks to have a look too.
Contributor
|
Would you mind also adding a CHANGELOG entry for the components package? |
Mamaduka
force-pushed
the
fix/base-control-visual-label
branch
from
d500ba8 to
d6c080d
Compare
andrewserong
approved these changes
Jan 10, 2022
Contributor
andrewserong
left a comment
andrewserong
left a comment
There was a problem hiding this comment.
Thanks for the ping @ciampo and for the PR @Mamaduka!
This tests well for me, the 8px bottom margin is consistent with the other label margins that I've looked at recently (e.g. the TextTransformControl), and I've tested this change with all the Typography controls that use the ToolsPanel and can't find any other obvious inconsistencies there.
I think the main other label margin that looks different is the SelectControl's label which currently uses a 4px bottom padding via the InputControl component here. It's visible on the Preload dropdown in the Video block:
But I think we could look at improving the consistency there in a separate PR if we'd like to follow-up on that component. This change LGTM!
Member
Author
|
Thanks for testing, @andrewserong and @ciampo. |
Contributor
aaronrobertshaw
left a comment
aaronrobertshaw
left a comment
There was a problem hiding this comment.
Looks like I was a few minutes too late to the party 🙂
This tested well for me. I couldn't spot any issues other than the minor difference with the SelectControl's label that @andrewserong mentioned. Everything else checked out for me in terms of various block supports' ToolsPanels and the Global Styles.
Thanks for putting this one together 👍
8 tasks
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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
PR adds missing styles for
BaseControl.VisualLabel. It was regression after #25842.Resolves #35785.
How has this been tested?
I tested based on issue instructions:
Screenshots
Types of changes
Regression
Checklist:
*.native.jsfiles for terms that need renaming or removal).