Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Global Styles Sidebar: Tweak spacing #40588

Merged
merged 1 commit into from
Apr 26, 2022
Merged

Global Styles Sidebar: Tweak spacing #40588

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 17 additions & 11 deletions packages/edit-site/src/components/global-styles/screen-root.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,8 @@
*/
import {
__experimentalItemGroup as ItemGroup,
__experimentalItem as Item,
__experimentalHStack as HStack,
__experimentalSpacer as Spacer,
__experimentalVStack as VStack,
FlexItem,
CardBody,
Expand Down Expand Up @@ -37,7 +37,7 @@ function ScreenRoot() {
return (
<Card size="small">
<CardBody>
<VStack spacing={ 2 }>
<VStack spacing={ 4 }>
<Card>
<CardMedia>
<StylesPreview />
Expand All @@ -55,22 +55,28 @@ function ScreenRoot() {
</HStack>
</NavigationButton>
) }
<ContextMenu />
</VStack>
</CardBody>

<CardBody>
<ContextMenu />
</CardBody>

<CardDivider />

<CardBody>
<Spacer
as="p"
paddingTop={ 2 }
/*
* 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"
Comment on lines +68 to +72
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

marginBottom={ 4 }
>
{ __(
'Customize the appearance of specific blocks for the whole site.'
) }
</Spacer>
<ItemGroup>
<Item>
{ __(
'Customize the appearance of specific blocks for the whole site.'
) }
</Item>
<NavigationButton path="/blocks">
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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 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!

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?)

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There was a conversation between @ItsJonQ and @diegohaz about the semantics of ItemGroup when it was first introduced:
#30097 (comment)

<HStack justify="space-between">
<FlexItem>{ __( 'Blocks' ) }</FlexItem>
Expand Down