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

Jump to bottom

Nav menu item enhancements: display toolbar and remove dropdown #17986

Merged
merged 11 commits into from
Oct 27, 2019
Merged

Nav menu item enhancements: display toolbar and remove dropdown #17986

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
45b3305
Display toolbar and remove dropdown from menu item
tellthemachines Oct 17, 2019
d0b9415
Fixes block toolbar misalignment on IE.
tellthemachines Oct 18, 2019
497dc91
Replace destination and deal with keypresses.
tellthemachines Oct 18, 2019
673aaf7
Update fixture.
tellthemachines Oct 21, 2019
9790db4
Keydown management and attempt at close on blur.
tellthemachines Oct 21, 2019
651e76e
Add definitive menu item icon.
tellthemachines Oct 22, 2019
30feecb
Fix label/input styling.
tellthemachines Oct 23, 2019
b8a66c6
Clean up styles after rebase.
tellthemachines Oct 23, 2019
7678977
Refactor stop propagation .
tellthemachines Oct 24, 2019
57a70fc
Remove duplicate dependency comments
noisysocks Oct 25, 2019
7e587cf
Navigation Block: Rename 'destination' to 'url' in server-side code
noisysocks Oct 25, 2019
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
2 changes: 2 additions & 0 deletions packages/block-editor/src/components/block-toolbar/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,8 @@
.block-editor-block-toolbar__slot {
// Required for IE11.
display: inline-block;
// Fix for toolbar button misalignment on IE11
line-height: 0;
draganescu marked this conversation as resolved.
Show resolved Hide resolved

// IE11 doesn't read rules inside this query. They are applied only to modern browsers.
@supports (position: sticky) {
Expand Down
6 changes: 3 additions & 3 deletions packages/block-library/src/navigation-menu-item/block.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,6 @@
"label": {
"type": "string"
},
"destination": {
"type": "string"
},
"nofollow": {
"type": "boolean",
"default": false
Expand All @@ -21,6 +18,9 @@
"opensInNewTab": {
"type": "boolean",
"default": false
},
"url": {
"type": "string"
Copy link
Member

Choose a reason for hiding this comment

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

It looks like we are already using the destination attribute to store the URL.

gutenberg/packages/block-library/src/navigation-menu/index.php

Line 33 in 013ed32

$html .= ' href="' . $menu_item['attrs']['destination'] . '"';

Copy link
Contributor

Choose a reason for hiding this comment

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

Destination sounds more poetic and if code is poetry why not. On the other hand, it is an URL property hahaha.

Copy link
Member

Choose a reason for hiding this comment

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

url seems to be more in line with other uses around WordPress

}
}
}
145 changes: 95 additions & 50 deletions packages/block-library/src/navigation-menu-item/edit.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,97 +1,142 @@
/**
* External dependencies
*/
import { invoke } from 'lodash';
/**
* External dependencies
*/
import classnames from 'classnames';

/**
* WordPress dependencies
*/
noisysocks marked this conversation as resolved.
Show resolved Hide resolved
/**
* WordPress dependencies
*/
import { withSelect } from '@wordpress/data';
import {
Dropdown,
ExternalLink,
IconButton,
PanelBody,
TextareaControl,
TextControl,
Toolbar,
ToggleControl,
ToolbarButton,
} from '@wordpress/components';
import {
LEFT,
RIGHT,
UP,
DOWN,
BACKSPACE,
ENTER,
} from '@wordpress/keycodes';
import { __ } from '@wordpress/i18n';
import {
BlockControls,
InnerBlocks,
InspectorControls,
PlainText,
URLPopover,
} from '@wordpress/block-editor';
import {
Fragment,
useCallback,
useRef,
useState,
} from '@wordpress/element';

/**
* Internal dependencies
*/
import MenuItemActions from './menu-item-actions';
const POPOVER_PROPS = { noArrow: true };

function NavigationMenuItemEdit( {
attributes,
clientId,
isSelected,
isParentOfSelectedBlock,
setAttributes,
} ) {
const plainTextRef = useRef( null );
const onEditLableClicked = useCallback(
( onClose ) => () => {
onClose();
invoke( plainTextRef, [ 'current', 'textarea', 'focus' ] );
},
[ plainTextRef ]
);
const [ isLinkOpen, setIsLinkOpen ] = useState( false );
const [ isEditingLink, setIsEditingLink ] = useState( false );
const [ urlInput, setUrlInput ] = useState( null );

const inputValue = urlInput !== null ? urlInput : url;

const onKeyDown = ( event ) => {
if ( [ LEFT, DOWN, RIGHT, UP, BACKSPACE, ENTER ].indexOf( event.keyCode ) > -1 ) {
// Stop the key event from propagating up to ObserveTyping.startTypingInTextField.
event.stopPropagation();
}
};

const closeURLPopover = () => {
setIsEditingLink( false );
setUrlInput( null );
setIsLinkOpen( false );
};

const autocompleteRef = useRef( null );

const onFocusOutside = ( event ) => {
const autocompleteElement = autocompleteRef.current;
if ( autocompleteElement && autocompleteElement.contains( event.target ) ) {
return;
}
closeURLPopover();
};

const { label, url } = attributes;
let content;
if ( isSelected ) {
content = (
<div className="wp-block-navigation-menu-item__edit-container">
<PlainText
ref={ plainTextRef }
className="wp-block-navigation-menu-item__field"
value={ attributes.label }
onChange={ ( label ) => setAttributes( { label } ) }
aria-label={ __( 'Navigation Label' ) }
maxRows={ 1 }
/>
<Dropdown
contentClassName="wp-block-navigation-menu-item__dropdown-content"
position="bottom left"
popoverProps={ POPOVER_PROPS }
renderToggle={ ( { isOpen, onToggle } ) => (
<IconButton
icon={ isOpen ? 'arrow-up-alt2' : 'arrow-down-alt2' }
label={ __( 'More options' ) }
onClick={ onToggle }
aria-expanded={ isOpen }
/>
) }
renderContent={ ( { onClose } ) => (
<MenuItemActions
clientId={ clientId }
destination={ attributes.destination }
onEditLableClicked={ onEditLableClicked( onClose ) }
/>
) }
/>
</div>
<TextControl
ref={ plainTextRef }
className="wp-block-navigation-menu-item__field"
value={ label }
onChange={ ( labelValue ) => setAttributes( { label: labelValue } ) }
label={ __( 'Navigation Label' ) }
hideLabelFromVision={ true }
/>
);
} else {
content = <div className="wp-block-navigation-menu-item__container">
{ attributes.label }
{ label }
</div>;
}

return (
<Fragment>
<BlockControls>
<Toolbar>
<ToolbarButton
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm working on the new Link UI implementation. It's so common to want a button to trigger the link UI that I'm thinking we'll end up having a dedicated button component which we will expose for this use case.

Something like <LinkControl.Toggle />.

Copy link
Contributor

Choose a reason for hiding this comment

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

@getdave Yes but with these things sometimes you end up not knowing which way to go, because it will still be very much possible to not use it ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it makes sense to have a toolbar link component that includes toggle and popover. As it is I'm repeating logic from the other instances of toolbar links, which is less than ideal.

name="link"
icon="admin-links"
title={ __( 'Link' ) }
onClick={ () => setIsLinkOpen( ! isLinkOpen ) }
/>
{ isLinkOpen &&
<>
<URLPopover
className="wp-block-navigation-menu-item__inline-link-input"
onClose={ closeURLPopover }
onFocusOutside={ onFocusOutside }
>
{ ( ! url || isEditingLink ) &&
<URLPopover.LinkEditor
value={ inputValue }
onChangeInputValue={ setUrlInput }
onKeyPress={ ( event ) => event.stopPropagation() }
Copy link
Contributor

Choose a reason for hiding this comment

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

As I mentioned in this comment, I think onKeyPress event is not already handled by the <URLPopover.LinkEditor />, and just in case this commit takes over of this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The keypress event handler passed into LinkEditor is attached to the form element, not to URLInput. Your commit shouldn't have any effect on this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry I got confused last night. Thanks @tellthemachines and sorry again to bring unnecessary noise here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No worries, always best to comment if you're not sure!

onKeyDown={ onKeyDown }
onSubmit={ ( event ) => event.preventDefault() }
autocompleteRef={ autocompleteRef }
/>
}
{ ( url && ! isEditingLink ) &&
<URLPopover.LinkViewer
onKeyPress={ ( event ) => event.stopPropagation() }
url={ url }
/>
}

</URLPopover>
</>
}
</Toolbar>
draganescu marked this conversation as resolved.
Show resolved Hide resolved
</BlockControls>
<InspectorControls>
<PanelBody
title={ __( 'Menu Settings' ) }
Expand Down
43 changes: 6 additions & 37 deletions packages/block-library/src/navigation-menu-item/editor.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,41 +1,14 @@
$menu-label-field-width: 140px;

.wp-block-navigation-menu-item__edit-container {
display: grid;
grid-auto-columns: min-content;
grid-auto-flow: column;
align-items: center;
white-space: nowrap;
border: 1px solid $light-gray-500;
// two pixes comes from two times one pixel border
width: $menu-label-field-width + $icon-button-size + 2px;
padding-left: 1px;
}

.wp-block-navigation-menu-item__edit-container .wp-block-navigation-menu-item__field {
border-right: 1px solid $light-gray-500 !important;
width: $menu-label-field-width;
border: none;
.wp-block-navigation-menu-item__field .components-text-control__input.components-text-control__input,
.wp-block-navigation-menu-item__container {
border-radius: 0;
padding-left: $grid-size-large;

min-height: $icon-button-size - 1px;
line-height: $icon-button-size - 1px;

&,
&:focus {
color: $dark-gray-500;
}
}

.wp-block-navigation-menu-item {
padding: $block-padding;
font-family: $editor-font;
font-weight: bold;
font-size: $text-editor-font-size;
background-color: var(--background-color-menu-link);
color: var(--color-menu-link);

.wp-block-navigation-menu-item__container,
&.is-editing .editor-plain-text {
background-color: var(--background-color-menu-link);
&:focus {
color: var(--color-menu-link);
}
}
Expand All @@ -61,10 +34,6 @@ $menu-label-field-width: 140px;
}

.wp-block-navigation-menu .block-editor-block-list__block[data-type="core/navigation-menu-item"] {
& > .block-editor-block-list__block-edit > div[role="toolbar"] {
display: none;
}

& > .block-editor-block-list__insertion-point {
display: none;
}
Expand Down
4 changes: 2 additions & 2 deletions packages/block-library/src/navigation-menu-item/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
* WordPress dependencies
*/
import { __ } from '@wordpress/i18n';

import { Path, SVG } from '@wordpress/components';
/**
* Internal dependencies
*/
Expand All @@ -18,7 +18,7 @@ export const settings = {

parent: [ 'core/navigation-menu' ],

icon: 'admin-links',
icon: <SVG xmlns="http://www.w3.org/2000/svg" width="24" height="24"><Path d="M12 7.27l4.28 10.43-3.47-1.53-.81-.36-.81.36-3.47 1.53L12 7.27M12 2L4.5 20.29l.71.71L12 18l6.79 3 .71-.71L12 2z" /></SVG>,

description: __( 'Add a page, link, or other item to your Navigation Menu.' ),

Expand Down
Loading