Global Styles Sidebar: Tweak spacing #40588
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
Add back description padding
mirka
added
Global Styles
|
Size Change: +16 B (0%) Total Size: 1.23 MB
ℹ️ View Unchanged
|
mirka
commented
Apr 25, 2022
| { __( | ||
| 'Customize the appearance of specific blocks for the whole site.' | ||
| ) } | ||
| </Item> | ||
| <NavigationButton path="/blocks"> |
There was a problem hiding this comment.
The use of Item here was problematic because it ignores the underlying HTML semantics (list and listitem).
Maybe a reminder for us to name components in a way that sufficiently evokes the HTML semantics? Another instance of semantic misuse is addressed in #40590.
There was a problem hiding this comment.
Yes, I agree that ItemGroup's name does not evoke at all the underlying list semantics.
I think that the issue with ItemGroup is that its main goal is to provide a coherent visual style for grouping/listing items, but it does so assuming that a good default is to implement HTML list semantics — while in our UIs, not everything that gets grouped together is necessary a list.
I can think of two approaches to improve the situation:
- we could have named the component more explicitly (i.e.
ItemList) - we could have made it just a visual component (e.g. without assuming any HTML semantics)
WDYT ?
There was a problem hiding this comment.
while in our UIs, not everything that gets grouped together is necessary a list.
I would probably argue that anything that's visually grouped together using ItemGroup is and should be a list. The ways I saw it misused (in Global Styles at least) were not legitimate use cases. So I still think it's a reasonable default for it to have list semantics.
- we could have named the component more explicitly (i.e.
ItemList)
This, absolutely. But also, I kind of think we should switch to using as="ul" instead of role="list". Then it would be possible to cleanly remove the semantics (however inadvisable that may be). As a plus, people could also make it an ol if they wanted!
There was a problem hiding this comment.
I would probably argue that anything that's visually grouped together using
ItemGroupis and should be a list. The ways I saw it misused (in Global Styles at least) were not legitimate use cases. So I still think it's a reasonable default for it to have list semantics.
In principle I agree with this. The only worry that I have is to avoid coupling semantics and visual look, but the rest of your reply also partially addresses that.
- we could have named the component more explicitly (i.e.
ItemList)This, absolutely. But also, I kind of think we should switch to using
as="ul"instead ofrole="list". Then it would be possible to cleanly remove the semantics (however inadvisable that may be). As a plus, people could also make it anolif they wanted!
This all makes sense. I'd be curious to know the original reasoning for using role instead of HTML elements in ItemGroup (@griffbrad maybe you have any context here?)
There was a problem hiding this comment.
There was a conversation between @ItsJonQ and @diegohaz about the semantics of ItemGroup when it was first introduced:
#30097 (comment)
ciampo
approved these changes
Apr 25, 2022
| { __( | ||
| 'Customize the appearance of specific blocks for the whole site.' | ||
| ) } | ||
| </Item> | ||
| <NavigationButton path="/blocks"> |
There was a problem hiding this comment.
Yes, I agree that ItemGroup's name does not evoke at all the underlying list semantics.
I think that the issue with ItemGroup is that its main goal is to provide a coherent visual style for grouping/listing items, but it does so assuming that a good default is to implement HTML list semantics — while in our UIs, not everything that gets grouped together is necessary a list.
I can think of two approaches to improve the situation:
- we could have named the component more explicitly (i.e.
ItemList) - we could have made it just a visual component (e.g. without assuming any HTML semantics)
WDYT ?
Comment on lines
+68
to
+72
| /* | ||
| * 13px matches the text inset of the NavigationButton (12px padding, plus the width of the button's border). | ||
| * This is an ad hoc override for this particular instance only and should be reconsidered before making into a pattern. | ||
| */ | ||
| paddingX="13px" |
There was a problem hiding this comment.
I personally don't like this approach, but given the conversation in #40533 (comment) and following, I'm ok to allow it as a rare exception.
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.
Part of #38934
What?
Cleans up spacing issues with the navigator buttons on the Global Styles Sidebar root screen.
Why?
To refine the visual consistency of the Global Styles sidebar.
How?
Tightened the main button spacing overall by optimizing the component tree structure, and then tweaked the explicit spacing values.
✅ Design review was done in #40533
Testing Instructions
npm run devScreenshots or screencast