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: Rename NavigationButton so semantics are clearer #40590

Merged
merged 2 commits into from
Apr 26, 2022
Merged

Global Styles Sidebar: Rename NavigationButton so semantics are clearer #40590

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0e3dbd3
PR 📌 Rename NavigationButton so semantics are clearer
mirka Apr 22, 2022
d879d78
Bandaid fix semantics in header back button
mirka Apr 22, 2022
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
14 changes: 7 additions & 7 deletions packages/edit-site/src/components/global-styles/context-menu.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ import { useHasBorderPanel } from './border-panel';
import { useHasColorPanel } from './color-utils';
import { useHasDimensionsPanel } from './dimensions-panel';
import { useHasTypographyPanel } from './typography-panel';
import { NavigationButton } from './navigation-button';
import { NavigationButtonAsItem } from './navigation-button';

function ContextMenu( { name, parentMenu = '' } ) {
const hasTypographyPanel = useHasTypographyPanel( name );
Expand All @@ -24,28 +24,28 @@ function ContextMenu( { name, parentMenu = '' } ) {
return (
<ItemGroup>
{ hasTypographyPanel && (
<NavigationButton
<NavigationButtonAsItem
icon={ typography }
path={ parentMenu + '/typography' }
>
{ __( 'Typography' ) }
</NavigationButton>
</NavigationButtonAsItem>
) }
{ hasColorPanel && (
<NavigationButton
<NavigationButtonAsItem
icon={ color }
path={ parentMenu + '/colors' }
>
{ __( 'Colors' ) }
</NavigationButton>
</NavigationButtonAsItem>
) }
{ hasLayoutPanel && (
<NavigationButton
<NavigationButtonAsItem
icon={ layout }
path={ parentMenu + '/layout' }
>
{ __( 'Layout' ) }
</NavigationButton>
</NavigationButtonAsItem>
) }
</ItemGroup>
);
Expand Down
6 changes: 3 additions & 3 deletions packages/edit-site/src/components/global-styles/header.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,14 +14,14 @@ import { chevronRight, chevronLeft } from '@wordpress/icons';
/**
* Internal dependencies
*/
import { NavigationBackButton } from './navigation-button';
import { NavigationBackButtonAsItem } from './navigation-button';

function ScreenHeader( { title, description } ) {
return (
<VStack spacing={ 2 }>
<HStack spacing={ 2 }>
<View>
<NavigationBackButton
<View role="list">
<NavigationBackButtonAsItem
Comment on lines +23 to +24
Copy link
Member Author

Choose a reason for hiding this comment

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

This bandaid fix will be ripped off in #40592.

icon={ isRTL() ? chevronRight : chevronLeft }
size="small"
aria-label={ __( 'Navigate to the previous view' ) }
Expand Down
6 changes: 3 additions & 3 deletions packages/edit-site/src/components/global-styles/navigation-button.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,12 +28,12 @@ function GenericNavigationButton( { icon, children, ...props } ) {
);
}

function NavigationButton( props ) {
function NavigationButtonAsItem( props ) {
return <NavigatorButton as={ GenericNavigationButton } { ...props } />;
}

function NavigationBackButton( props ) {
function NavigationBackButtonAsItem( props ) {
return <NavigatorBackButton as={ GenericNavigationButton } { ...props } />;
}

export { NavigationButton, NavigationBackButton };
export { NavigationButtonAsItem, NavigationBackButtonAsItem };
6 changes: 3 additions & 3 deletions packages/edit-site/src/components/global-styles/palette.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ import { useMemo } from '@wordpress/element';
* Internal dependencies
*/
import Subtitle from './subtitle';
import { NavigationButton } from './navigation-button';
import { NavigationButtonAsItem } from './navigation-button';
import { useSetting } from './hooks';
import ColorIndicatorWrapper from './color-indicator-wrapper';

Expand Down Expand Up @@ -58,7 +58,7 @@ function Palette( { name } ) {
<VStack spacing={ 3 }>
<Subtitle>{ __( 'Palette' ) }</Subtitle>
<ItemGroup isBordered isSeparated>
<NavigationButton path={ screenPath }>
<NavigationButtonAsItem path={ screenPath }>
<HStack
direction={
colors.length === 0 ? 'row-reverse' : 'row'
Expand All @@ -73,7 +73,7 @@ function Palette( { name } ) {
</ZStack>
<FlexItem>{ paletteButtonText }</FlexItem>
</HStack>
</NavigationButton>
</NavigationButtonAsItem>
</ItemGroup>
</VStack>
);
Expand Down
6 changes: 3 additions & 3 deletions packages/edit-site/src/components/global-styles/screen-block-list.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ import { useHasColorPanel } from './color-utils';
import { useHasDimensionsPanel } from './dimensions-panel';
import { useHasTypographyPanel } from './typography-panel';
import ScreenHeader from './header';
import { NavigationButton } from './navigation-button';
import { NavigationButtonAsItem } from './navigation-button';

function useSortedBlockTypes() {
const blockItems = useSelect(
Expand Down Expand Up @@ -61,12 +61,12 @@ function BlockMenuItem( { block } ) {
}

return (
<NavigationButton path={ '/blocks/' + block.name }>
<NavigationButtonAsItem path={ '/blocks/' + block.name }>
<HStack justify="flex-start">
<BlockIcon icon={ block.icon } />
<FlexItem>{ block.title }</FlexItem>
</HStack>
</NavigationButton>
</NavigationButtonAsItem>
);
}

Expand Down
14 changes: 7 additions & 7 deletions packages/edit-site/src/components/global-styles/screen-colors.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ import {
*/
import ScreenHeader from './header';
import Palette from './palette';
import { NavigationButton } from './navigation-button';
import { NavigationButtonAsItem } from './navigation-button';
import { getSupportedGlobalStylesPanels, useStyle } from './hooks';
import Subtitle from './subtitle';
import ColorIndicatorWrapper from './color-indicator-wrapper';
Expand All @@ -33,7 +33,7 @@ function BackgroundColorItem( { name, parentMenu } ) {
}

return (
<NavigationButton path={ parentMenu + '/colors/background' }>
<NavigationButtonAsItem path={ parentMenu + '/colors/background' }>
<HStack justify="flex-start">
<ColorIndicatorWrapper expanded={ false }>
<ColorIndicator
Expand All @@ -42,7 +42,7 @@ function BackgroundColorItem( { name, parentMenu } ) {
</ColorIndicatorWrapper>
<FlexItem>{ __( 'Background' ) }</FlexItem>
</HStack>
</NavigationButton>
</NavigationButtonAsItem>
);
}

Expand All @@ -56,14 +56,14 @@ function TextColorItem( { name, parentMenu } ) {
}

return (
<NavigationButton path={ parentMenu + '/colors/text' }>
<NavigationButtonAsItem path={ parentMenu + '/colors/text' }>
<HStack justify="flex-start">
<ColorIndicatorWrapper expanded={ false }>
<ColorIndicator colorValue={ color } />
</ColorIndicatorWrapper>
<FlexItem>{ __( 'Text' ) }</FlexItem>
</HStack>
</NavigationButton>
</NavigationButtonAsItem>
);
}

Expand All @@ -77,14 +77,14 @@ function LinkColorItem( { name, parentMenu } ) {
}

return (
<NavigationButton path={ parentMenu + '/colors/link' }>
<NavigationButtonAsItem path={ parentMenu + '/colors/link' }>
<HStack justify="flex-start">
<ColorIndicatorWrapper expanded={ false }>
<ColorIndicator colorValue={ color } />
</ColorIndicatorWrapper>
<FlexItem>{ __( 'Links' ) }</FlexItem>
</HStack>
</NavigationButton>
</NavigationButtonAsItem>
);
}

Expand Down
30 changes: 17 additions & 13 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 @@ -21,7 +21,7 @@ import { store as coreStore } from '@wordpress/core-data';
* Internal dependencies
*/
import { IconWithCurrentColor } from './icon-with-current-color';
import { NavigationButton } from './navigation-button';
import { NavigationButtonAsItem } from './navigation-button';
import ContextMenu from './context-menu';
import StylesPreview from './preview';

Expand All @@ -44,16 +44,20 @@ function ScreenRoot() {
</CardMedia>
</Card>
{ !! variations?.length && (
<NavigationButton path="/variations">
<HStack justify="space-between">
<FlexItem>{ __( 'Browse styles' ) }</FlexItem>
<IconWithCurrentColor
icon={
isRTL() ? chevronLeft : chevronRight
}
/>
</HStack>
</NavigationButton>
<ItemGroup>
<NavigationButtonAsItem path="/variations">
Comment on lines +47 to +48
Copy link
Member Author

Choose a reason for hiding this comment

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

This "fixes" the semantic issue, but wrapping this single item in an ItemGroup is not ideal. Ideally I would like to use a vanilla NavigatorButton here instead of the Item-wrapped version, but unfortunately there is a styling inconsistency (#40591) that makes that weird too.

I think we can go with this bandaid fix, since we'll probably overhaul this view (#40478) anyway.

Copy link
Contributor

Choose a reason for hiding this comment

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

What about overriding the role, so that at least we don't have a semantically weird one-item list ?

Suggested change
<ItemGroup>
<NavigationButtonAsItem path="/variations">
<ItemGroup role={ null }>
<NavigationButtonAsItem
path="/variations"
role={ null }
>

Just a caveat: eslint complains about passing null as a value, but it's the only way to properly override this value (passing undefined would make ItemGroup and Item fallback on their default role values)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I'm hesitant to do that too since I agree with eslint that role={ null } is not at all kosher 😅 And neither is role="generic", apparently.

My preference would be to fix the focus outline inconsistency so we can just use a vanilla NavigatorButton. (A potential path to overriding semantics cleanly is the as prop though.)

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed. Let's improve the focus outline inconsistency, and then look at a way to rewrite ItemGroup in a way that makes possible using as to override semantics (together with a few other tweaks that we took note of)

<HStack justify="space-between">
<FlexItem>
{ __( 'Browse styles' ) }
</FlexItem>
<IconWithCurrentColor
icon={
isRTL() ? chevronLeft : chevronRight
}
/>
</HStack>
</NavigationButtonAsItem>
</ItemGroup>
) }
<ContextMenu />
</VStack>
Expand All @@ -77,14 +81,14 @@ function ScreenRoot() {
) }
</Spacer>
<ItemGroup>
<NavigationButton path="/blocks">
<NavigationButtonAsItem path="/blocks">
<HStack justify="space-between">
<FlexItem>{ __( 'Blocks' ) }</FlexItem>
<IconWithCurrentColor
icon={ isRTL() ? chevronLeft : chevronRight }
/>
</HStack>
</NavigationButton>
</NavigationButtonAsItem>
</ItemGroup>
</CardBody>
</Card>
Expand Down
6 changes: 3 additions & 3 deletions packages/edit-site/src/components/global-styles/screen-typography.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ import {
* Internal dependencies
*/
import ScreenHeader from './header';
import { NavigationButton } from './navigation-button';
import { NavigationButtonAsItem } from './navigation-button';
import { useStyle } from './hooks';
import Subtitle from './subtitle';
import TypographyPanel from './typography-panel';
Expand Down Expand Up @@ -44,7 +44,7 @@ function Item( { name, parentMenu, element, label } ) {
}

return (
<NavigationButton path={ parentMenu + '/typography/' + element }>
<NavigationButtonAsItem path={ parentMenu + '/typography/' + element }>
<HStack justify="flex-start">
<FlexItem
className="edit-site-global-styles-screen-typography__indicator"
Expand All @@ -62,7 +62,7 @@ function Item( { name, parentMenu, element, label } ) {
</FlexItem>
<FlexItem>{ label }</FlexItem>
</HStack>
</NavigationButton>
</NavigationButtonAsItem>
);
}

Expand Down