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.

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

Edit Post: Refactor More (Ellipsis) Menu #5161

Merged
merged 10 commits into from
Feb 22, 2018
1 change: 1 addition & 0 deletions components/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ export { default as FormToggle } from './form-toggle';
export { default as FormTokenField } from './form-token-field';
export { default as IconButton } from './icon-button';
export { default as KeyboardShortcuts } from './keyboard-shortcuts';
export { default as MenuItemsChoice } from './menu-items/menu-items-choice';
export { default as MenuItemsGroup } from './menu-items/menu-items-group';
export { default as MenuItemsToggle } from './menu-items/menu-items-toggle';
export { NavigableMenu, TabbableContainer } from './navigable-container';
Expand Down
28 changes: 28 additions & 0 deletions components/menu-items/menu-items-choice.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
/**
* Internal dependencies
*/
import './style.scss';
import MenuItemsToggle from './menu-items-toggle';

export default function MenuItemsChoice( {
choices = [],
onSelect,
value,
} ) {
return choices.map( ( item ) => {
const isSelected = value === item.value;
return (
<MenuItemsToggle
key={ item.value }
label={ item.label }
isSelected={ isSelected }
shortcut={ item.shortcut }
onClick={ () => {
if ( ! isSelected ) {
onSelect( item.value );
}
} }
/>
);
} );
}
50 changes: 23 additions & 27 deletions components/menu-items/menu-items-group.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,49 +3,45 @@
*/
import classnames from 'classnames';

/**
* WordPress dependencies
*/
import { Children } from '@wordpress/element';
import { applyFilters } from '@wordpress/hooks';

/**
* Internal dependencies
*/
import './style.scss';
import { NavigableMenu } from '../navigable-container';
import withInstanceId from '../higher-order/with-instance-id';
import MenuItemsToggle from './menu-items-toggle';

function MenuItemsGroup( {
label,
value,
choices = [],
onSelect,
export function MenuItemsGroup( {
children,
instanceId,
className = '',
filterName,
instanceId,
label,
} ) {
const labelId = `components-choice-menu-label-${ instanceId }`;
const classNames = classnames( className, 'components-choice-menu' );
const childrenArray = Children.toArray( children );
const menuItems = filterName ?
applyFilters( filterName, childrenArray ) :
childrenArray;

if ( ! Array.isArray( menuItems ) || ! menuItems.length ) {
return null;
}

const labelId = `components-menu-items-group-label-${ instanceId }`;
const classNames = classnames( className, 'components-menu-items-group' );

return (
<div className={ classNames }>
{ label &&
<div className="components-choice-menu__label" id={ labelId }>{ label }</div>
<div className="components-menu-items-group__label" id={ labelId }>{ label }</div>
}
<NavigableMenu orientation="vertical" aria-labelledby={ labelId }>
{ choices.map( ( item ) => {
const isSelected = value === item.value;
return (
<MenuItemsToggle
key={ item.value }
label={ item.label }
isSelected={ isSelected }
shortcut={ item.shortcut }
onClick={ () => {
if ( ! isSelected ) {
onSelect( item.value );
}
} }
/>
);
} ) }
{ children }
{ menuItems }
</NavigableMenu>
</div>
);
Expand Down
4 changes: 2 additions & 2 deletions components/menu-items/style.scss
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
.components-choice-menu {
.components-menu-items-group {
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Updating

Copy link
Member Author

Choose a reason for hiding this comment

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

Opened #5218 to address it.

width: 100%;
padding: 10px;
}

.components-choice-menu__label {
.components-menu-items-group__label {
margin-bottom: 10px;
color: $dark-gray-300;
}
Expand Down
24 changes: 24 additions & 0 deletions components/menu-items/test/__snapshots__/menu-items-group.js.snap
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
// Jest Snapshot v1, https://goo.gl/fbAQLP

exports[`MenuItemsGroup should match snapshot 1`] = `
<div
className="components-menu-items-group"
>
<div
className="components-menu-items-group__label"
id="components-menu-items-group-label-1"
>
My group
</div>
<NavigableMenu
aria-labelledby="components-menu-items-group-label-1"
orientation="vertical"
>
<p
key=".0"
>
My item
</p>
</NavigableMenu>
</div>
`;
30 changes: 30 additions & 0 deletions components/menu-items/test/menu-items-group.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
/**
* External dependencies
*/
import { shallow } from 'enzyme';

/**
* Internal dependencies
*/
import { MenuItemsGroup } from '../menu-items-group';

describe( 'MenuItemsGroup', () => {
test( 'should render null when no children provided', () => {
const wrapper = shallow( <MenuItemsGroup /> );

expect( wrapper.html() ).toBe( null );
} );

test( 'should match snapshot', () => {
const wrapper = shallow(
<MenuItemsGroup
label="My group"
instanceId="1"
>
<p>My item</p>
</MenuItemsGroup>
);

expect( wrapper ).toMatchSnapshot();
} );
} );
18 changes: 0 additions & 18 deletions edit-post/components/header/editor-actions/index.js

This file was deleted.

20 changes: 0 additions & 20 deletions edit-post/components/header/ellipsis-menu/style.scss

This file was deleted.

1 change: 1 addition & 0 deletions edit-post/components/header/fixed-toolbar-toggle/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ function FeatureToggle( { onToggle, active, onMobile } ) {
return (
<MenuItemsGroup
label={ __( 'Settings' ) }
filterName="editPost.MoreMenu.settings"
>
<MenuItemsToggle
label={ __( 'Fix Toolbar to Top' ) }
Expand Down
4 changes: 2 additions & 2 deletions edit-post/components/header/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ import {
* Internal dependencies
*/
import './style.scss';
import EllipsisMenu from './ellipsis-menu';
import MoreMenu from './more-menu';
import HeaderToolbar from './header-toolbar';
import { isSidebarOpened } from '../../store/selectors';
import { toggleSidebar } from '../../store/actions';
Expand Down Expand Up @@ -52,7 +52,7 @@ function Header( {
label={ __( 'Settings' ) }
aria-expanded={ isDefaultSidebarOpened }
/>
<EllipsisMenu key="ellipsis-menu" />
<MoreMenu key="more-menu" />
</div>
) }
</div>
Expand Down
14 changes: 9 additions & 5 deletions edit-post/components/header/mode-switcher/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import { connect } from 'react-redux';
* WordPress dependencies
*/
import { __ } from '@wordpress/i18n';
import { MenuItemsGroup } from '@wordpress/components';
import { MenuItemsChoice, MenuItemsGroup } from '@wordpress/components';

/**
* Internal dependencies
Expand Down Expand Up @@ -43,10 +43,14 @@ function ModeSwitcher( { onSwitch, mode } ) {
return (
<MenuItemsGroup
label={ __( 'Editor' ) }
choices={ choices }
value={ mode }
onSelect={ onSwitch }
/>
filterName="editPost.MoreMenu.editor"
>
<MenuItemsChoice
choices={ choices }
value={ mode }
onSelect={ onSwitch }
/>
</MenuItemsGroup>
);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,19 +2,18 @@
* WordPress dependencies
*/
import { __ } from '@wordpress/i18n';
import { IconButton, Dropdown } from '@wordpress/components';
import { IconButton, Dropdown, MenuItemsGroup } from '@wordpress/components';

/**
* Internal dependencies
*/
import './style.scss';
import ModeSwitcher from '../mode-switcher';
import FixedToolbarToggle from '../fixed-toolbar-toggle';
import EditorActions from '../editor-actions';

const element = (
const MoreMenu = () => (
Copy link
Member

Choose a reason for hiding this comment

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

Did this need to be changed to a function?

See also an explanation of this type of optimization: https://babeljs.io/docs/plugins/transform-react-constant-elements/

Copy link
Member Author

@gziolo gziolo Feb 23, 2018

Choose a reason for hiding this comment

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

That's interesting. I haven't seen it before in the React docs.

This transform should be enabled only in production (e.g., just before minifying your code) because although it improves runtime performance, it makes warning messages more cryptic.

Can we enable the transform instead and leave all the low-level optimizations to Babel?

<Dropdown
className="edit-post-ellipsis-menu"
className="edit-post-more-menu"
position="bottom left"
renderToggle={ ( { isOpen, onToggle } ) => (
<IconButton
Expand All @@ -25,19 +24,16 @@ const element = (
/>
) }
renderContent={ ( { onClose } ) => (
<div>
<div className="edit-post-more-menu__content">
<ModeSwitcher onSelect={ onClose } />
<div className="edit-post-ellipsis-menu__separator" />
<FixedToolbarToggle onToggle={ onClose } />
<div className="edit-post-ellipsis-menu__separator" />
<EditorActions />
<MenuItemsGroup
label={ __( 'Tools' ) }
filterName="editPost.MoreMenu.tools"
Copy link
Member

Choose a reason for hiding this comment

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

Conceptually-speaking, why do we need two things - plugin slots and filterable menus - to express the same idea of "add to this menu" ? As of current master, we have both in this menu, and it's not clear why I would use one or the other, or why one can't be satisfied by the other.

Copy link
Member

Choose a reason for hiding this comment

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

<PluginMoreMenuGroup.Slot fillProps={ { onClose } } />
<MenuGroup
label={ __( 'Tools' ) }
filterName="editPost.MoreMenu.tools"
>

Copy link
Contributor

Choose a reason for hiding this comment

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

My guess is filterable menus were some of the earlier experiments, partly motivated by the emergence of @wordpress/hooks. They also felt cheaper (perhaps deceptively simple?). I'd say by now slots have established themselves as the standard extension method, though.

Copy link
Contributor

Choose a reason for hiding this comment

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

cc @gziolo I think he was talking about removing those at some point?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, we should remove those filters and replace the only occurrence of hook based extension with slot and fill. As @mcsf already pointed out, this was an early exploration which turned out to be suboptimal from developer’s standpoint.

I had it planned to refactor on my todos list but it was never a priority. I will take care of it this week to avoid confusion.

/>
</div>
) }
/>
);

function EllipsisMenu() {
return element;
}

export default EllipsisMenu;
export default MoreMenu;
21 changes: 21 additions & 0 deletions edit-post/components/header/more-menu/style.scss
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
.edit-post-more-menu {
// the padding and margin of the more menu is intentionally non-standard
@include break-small() {
margin-left: 4px;
}

.components-icon-button {
padding: 8px 4px;
width: auto;
}

.components-button svg {
transform: rotate(90deg);
}
}

.edit-post-more-menu__content {
.components-menu-items-group:not(:last-child) {
border-bottom: 1px solid $light-gray-500;
}
}
2 changes: 1 addition & 1 deletion editor/hooks/index.js → edit-post/hooks/index.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
/**
* Internal dependencies
*/
import './copy-content';
import './more-menu';
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,9 @@
import { ClipboardButton, withState } from '@wordpress/components';
import { compose } from '@wordpress/element';
import { query } from '@wordpress/data';
import { addFilter } from '@wordpress/hooks';
import { __ } from '@wordpress/i18n';

function CopyContentButton( { editedPostContent, hasCopied, setState } ) {
function CopyContentMenuItem( { editedPostContent, hasCopied, setState } ) {
return (
<ClipboardButton
text={ editedPostContent }
Expand All @@ -22,17 +21,9 @@ function CopyContentButton( { editedPostContent, hasCopied, setState } ) {
);
}

const Enhanced = compose(
export default compose(
query( ( select ) => ( {
editedPostContent: select( 'core/editor' ).getEditedPostAttribute( 'content' ),
} ) ),
withState( { hasCopied: false } )
)( CopyContentButton );

const buttonElement = <Enhanced key="copy-content-button" />;

addFilter(
'editor.EditorActions.tools',
'core/copy-content/button',
( children ) => [ ...children, buttonElement ]
);
)( CopyContentMenuItem );
Loading