Global Styles Sidebar: adjust icon vertical alignment with labels #39982
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
ciampo
added
[Type] Enhancement
|
Size Change: +70 B (0%) Total Size: 1.22 MB
ℹ️ View Unchanged
|
jasmussen
reviewed
Apr 4, 2022
jasmussen
reviewed
Apr 4, 2022
|
Nice one, this benefits the blocks list as well: Happy to ✅ this one, although left some comments. Finally a separate question (not for this PR). We have a lot of places where we show icon+label like this. Should these all use a similar component or are there any generic opportunities here?
In all those cases, the icon size, space between icon and label, as well as font size/font should be identical. |
This topic was discussed in #36653, and at the time it was decided to keep the lower-level Given what was discussed and agreed, I suggest we create such a component in a more "local" environment (for example, just for the Global Styles sidebar, or maybe in the |
mirka
approved these changes
Apr 4, 2022
Comment on lines
99
to
100
| margin-top: $grid-unit * 0.25; | ||
| margin-bottom: $grid-unit * 0.25; |
There was a problem hiding this comment.
Just a nit, but I find that a "flexed wrapper div set to 24px height" captures the intent better. Or at least a wrapper div with vertical padding, rather than margins on ColorIndicator itself.
There was a problem hiding this comment.
Good point. I'm going to close this conversation as I've decided to adopt an approach based on your suggestion in this comment
There was a problem hiding this comment.
I actually went for this approach in the end, and added a ColorIndicatorWrapper component which is basically a Flex component with a fixed height of 24px
…Indicator` usages
ciampo
force-pushed
the
fix/global-styles-sidebar-icons-alignment
branch
from
dcca723
to
d33b9a4
Compare
7 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.
What?
This PR adjusts the vertical alignment between icons and labels in the Global Styles sidebar, as flagged in #38934.
HStack,FlexandItemGroup, in case some of the anti-patterns described below are used.Why?
This is in order to follow the design specs highlighted in #38934
How?
After inspecting the layout of icons and labels, I noticed that the browser was always adding, under each icon, a few px of white space — this was due to the fact that the icons are an inline-block element within a block element, and therefore that white space was automatically added by the browser. Notice how the icon's wrapper element has a computed height of
27px, instead of the expected24px:This is a "classic" CSS layout problem (one hacky way to fix it, for example, is to set the
font-sizeof the parent container to0). Instead, I spent some more time in order to come up with a better fix:FlexItem/FlexBlockwrappers aroundIconcomponents (FlexItem/FlexBlockhavedisplay: block, and were not really being of any use. By removing them, the new parent element of the icon becomes thedisplay: flexparent, which automatically ignores the white space that previously was causing the misalignment)display: flexparents, I had to apply those styles to theZStackcomponent to add that same fix the color palette screen (I will merge the changes toZStackseparately, but for now I've kept those changes within this PR for ease of understanding)4pxshorter with respect to all other items. This is because<ColorIndicator />has a size of20px, while the rest of the icons have a size of24px. To make up for this, I've decided to apply a2pxtop and bottom margin around the<ColorIndicator />s, but we could also look into adding the possibility for<ColorIndicator />to have a size of24pxinstead (this needs design feedback)Testing Instructions
Screenshots or screencast
Example of the changes on the root screen: