Skip to content

Navigation: Experiment with Divider component for setup state #36302

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

Closed
jasmussen wants to merge 1 commit into from
Closed

Navigation: Experiment with Divider component for setup state #36302

Show file tree
Hide file tree
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
43 changes: 33 additions & 10 deletions packages/block-library/src/navigation/edit/placeholder/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,17 +8,17 @@ import {
DropdownMenu,
MenuGroup,
MenuItem,
__experimentalDivider as Divider,
} from '@wordpress/components';
import { store as coreStore } from '@wordpress/core-data';
import { useDispatch } from '@wordpress/data';
import { useCallback, useState, useEffect } from '@wordpress/element';
import { __ } from '@wordpress/i18n';
import { navigation, chevronDown, Icon } from '@wordpress/icons';
import { navigation, Icon } from '@wordpress/icons';

/**
* Internal dependencies
*/

import useNavigationEntities from '../../use-navigation-entities';
import PlaceholderPreview from './placeholder-preview';
import menuItemsToBlocks from '../../menu-items-to-blocks';
Expand All @@ -34,15 +34,16 @@ const ExistingMenusDropdown = ( {
onCreateFromMenu,
} ) => {
const toggleProps = {
variant: 'primary',
variant: 'tertiary',
iconPosition: 'right',
className: 'wp-block-navigation-placeholder__actions__dropdown',
};
return (
<DropdownMenu
text={ __( 'Select existing menu' ) }
icon={ chevronDown }
text={ __( 'Existing menu' ) }
toggleProps={ toggleProps }
popoverProps={ { isAlternate: true } }
icon={ null }
>
{ ( { onClose } ) => (
<>
Expand Down Expand Up @@ -187,6 +188,20 @@ export default function NavigationPlaceholder( {

const { navigationMenus } = useNavigationMenu();

const dividerProps = {
orientation: 'vertical',
};
const dividerStyles = {
Copy link
Contributor Author

@jasmussen jasmussen Nov 8, 2021

Choose a reason for hiding this comment

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

If I set the orientation to vertical, it seems like the component could be smart enough that the only styles I needed to define were height (if I wanted anything other than auto), and possibly left/right margins. It seems like we could provide nice defaults for margins and colors so those would be accurate without overriding in most cases.

borderColor: 'black',
borderWidth: '0 1px 0 0',
borderStyle: 'solid',
width: '1px',
height: '18px',
marginTop: 'auto',
marginBottom: 'auto',
marginRight: '11px',
};
Comment on lines +191 to +203
Copy link
Contributor

@ciampo ciampo Nov 12, 2021

Choose a reason for hiding this comment

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

Just as a FYI, it's usually better to avoid inline styles, and use classes instead!

Having said that, another FYI is that style can be passed directly as a prop:

const dividerStyles={ ... };

const dividerProps = {
	orientation: 'vertical',
	style: dividerStyles,
};

Copy link
Contributor Author

@jasmussen jasmussen Nov 15, 2021

Choose a reason for hiding this comment

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

Stellar.


return (
<>
{ ( ! hasResolvedNavigationMenus || isStillLoading ) && (
Expand All @@ -201,6 +216,10 @@ export default function NavigationPlaceholder( {
<Icon icon={ navigation } />{ ' ' }
{ __( 'Navigation' ) }
</div>
<Divider
{ ...dividerProps }
style={ dividerStyles }
/>
{ hasMenus || navigationMenus.length ? (
<ExistingMenusDropdown
canSwitchNavigationMenu={
Expand All @@ -213,13 +232,13 @@ export default function NavigationPlaceholder( {
onCreateFromMenu={ onCreateFromMenu }
/>
) : undefined }
<Divider
{ ...dividerProps }
style={ dividerStyles }
/>
{ hasPages ? (
<Button
variant={
hasMenus || canSwitchNavigationMenu
? 'tertiary'
: 'primary'
}
variant="tertiary"
onClick={ () => {
setIsNewMenuModalVisible( true );
setCreateEmpty( false );
Expand All @@ -228,6 +247,10 @@ export default function NavigationPlaceholder( {
{ __( 'Add all pages' ) }
</Button>
) : undefined }
<Divider
{ ...dividerProps }
style={ dividerStyles }
/>
<Button
variant="tertiary"
onClick={ () => {
Expand Down
9 changes: 1 addition & 8 deletions packages/block-library/src/navigation/editor.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -386,21 +386,14 @@ $color-control-label-height: 20px;
font-size: $default-font-size;
font-family: $default-font;

.components-button.components-dropdown-menu__toggle.has-icon {
padding:
( $grid-unit-15 * 0.5 ) $grid-unit-05 ( $grid-unit-15 * 0.5 )
$grid-unit-15;
display: flex;
flex-direction: row-reverse; // This puts the chevron, which is hidden from screen readers, on the right.
}

// Margins.
.components-dropdown,
> .components-button {
margin-right: $grid-unit-15;
}
}


/**
* Mobile menu.
*/
Expand Down