FontSizePicker: Add flag to remove bottom margin #43062
Merged
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: +122 B (0%) Total Size: 1.27 MB
ℹ️ View Unchanged
|
mirka
force-pushed
the
font-size-picker-no-margin
branch
from
be609ba to
6bc2f8e
Compare
This was referenced Aug 8, 2022
mirka
commented
Aug 8, 2022
| @@ -212,6 +213,7 @@ function UnforwardedRangeControl< IconProps = unknown >( | |||
|
|
|||
| return ( | |||
| <BaseControl | |||
There was a problem hiding this comment.
@ciampo There is an ever so slight space here that I can only resolve by setting a line-height: 0 on .components-base-control__field, and I don't know what is happening 😂 Let me know if you have any ideas.
There was a problem hiding this comment.
That's the typical "white space" issue that happens around inline elements — there are different ways to fix, including setting font-size: 0 or line-height: 0 on .components-base-control__field.
But in my opinion those are just "patchy" fixes — a better way to handle it is to avoid having inline elements, which cause the white space in the first place.
The easiest fix would be to change RangeControl's Root element from display: inline-flex to display: flex.
In case there was a reason for that Root element to be inline-flex and we don't want to touch it, another alternative is to set display: flex; flex-direction: column on .components-base-control__field (also in this scenario, we'd need to check that this change doesn't introduce any regression).
Bonus round: for both proposed solutions, we could use HStack or VStack instead!
There was a problem hiding this comment.
I had so much trouble finding the culprit, thank you 😂
The easiest fix would be to change
RangeControl'sRootelement fromdisplay: inline-flextodisplay: flex.
I went with this fix — it looks safe from what I could gather.
ciampo
reviewed
Aug 10, 2022
ciampo
reviewed
Aug 10, 2022
It was 😱 I'm so glad there was a test that caught it, it would've been very bad if that was merged 😱😱 9db8f69 Conditional Components: A Cautionary Tale |
ciampo
approved these changes
Aug 11, 2022
There was a problem hiding this comment.
LGTM 🚀
I only noticed still a little glimpse of a white space in the RangeControl component when adding a before/after icon
Kapture.2022-08-11.at.18.12.52.mp4
It should be fixable by setting display: flex on AfterIconWrapper and BeforeIconWrapper (but I haven't double-checked) 🤞
Feel free to merge once this small details is taken care of :)
mirka
force-pushed
the
font-size-picker-no-margin
branch
from
a313a01 to
aa8d839
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.
Closes #38720
Part of #39358
What?
Adds a
__nextHasNoMarginBottomprop to the following components, to opt into the new margin-free styles.Why?
For easier reuse and consistency in spacing.
How?
This PR just adds the flags. The in-repo migration and official deprecation tasks will continue to be tracked at #39358.
Testing Instructions
Apply this patch for easier testing in Storybook (it injects a colored div after the component):
npm run storybook:devCheck the stories for ToggleGroupControl, RangeControl, and FontSizePicker.