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

Plugins: Add pinning support for sidebar plugins #6442

Merged
merged 14 commits into from
May 14, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
21 changes: 19 additions & 2 deletions edit-post/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ const MyPluginSidebar = () => (
<PluginSidebar
name="my-sidebar"
title="My sidebar title"
icon="smiley"
>
<PanelBody>
{ __( 'My sidebar content' ) }
Expand All @@ -53,6 +54,22 @@ Title displayed at the top of the sidebar.
- Type: `String`
- Required: Yes

##### isPinnable

Whether to allow to pin sidebar to toolbar.

- Type: `Boolean`
- Required: No
- Default: `true`

##### icon
Copy link
Member

Choose a reason for hiding this comment

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

Should this be a property of registerPlugin rather than a prop of PluginSidebar ?

Copy link
Member Author

Choose a reason for hiding this comment

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

As I had a similar dilemma. I will explore how much work is required to move it to registerPlugin.

Copy link
Member

Choose a reason for hiding this comment

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

I mean, even just as a default, it seems like many / most plugins would have a single icon to represent themselves throughout the entire experience.

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, I like it. It should work. I’ll update PR on Monday 👍

Copy link
Member Author

Choose a reason for hiding this comment

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

I added an option to provide an icon property as part of plugin's registration with 15a51cc. It should give it more flexibility because I left the capability to override this icon for the individual components.


The [Dashicon](https://developer.wordpress.org/resource/dashicons/) icon slug string, or an SVG WP element, to be rendered when the sidebar is pinned to toolbar.

- Type: `String` | `Element`
- Required: No
- Default: _inherits from the plugin_


### `PluginSidebarMoreMenuItem`

Expand All @@ -68,7 +85,7 @@ const { PluginSidebarMoreMenuItem } = wp.editPost;
const MySidebarMoreMenuItem = () => (
<PluginSidebarMoreMenuItem
target="my-sidebar"
icon="yes"
icon="smiley"
>
{ __( 'My sidebar title' ) }
</PluginSidebarMoreMenuItem>
Expand All @@ -90,7 +107,7 @@ The [Dashicon](https://developer.wordpress.org/resource/dashicons/) icon slug st

- Type: `String` | `Element`
- Required: No

- Default: _inherits from the plugin_

### `PluginPostStatusInfo`

Expand Down
6 changes: 4 additions & 2 deletions edit-post/components/header/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import { compose } from '@wordpress/element';
import './style.scss';
import MoreMenu from './more-menu';
import HeaderToolbar from './header-toolbar';
import PinnedPlugins from './pinned-plugins';

function Header( {
isEditorSidebarOpened,
Expand Down Expand Up @@ -52,12 +53,13 @@ function Header( {
/>
<IconButton
icon="admin-generic"
label={ __( 'Settings' ) }
onClick={ toggleGeneralSidebar }
isToggled={ isEditorSidebarOpened }
label={ __( 'Settings' ) }
aria-expanded={ isEditorSidebarOpened }
/>
<MoreMenu key="more-menu" />
<PinnedPlugins.Slot />
<MoreMenu />
</div>
) }
</div>
Expand Down
28 changes: 28 additions & 0 deletions edit-post/components/header/pinned-plugins/index.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
/**
* External dependencies
*/
import { isEmpty } from 'lodash';

/**
* WordPress dependencies
*/
import { createSlotFill } from '@wordpress/components';

/**
* Internal dependencies
*/
import './style.scss';

const { Fill: PinnedPlugins, Slot } = createSlotFill( 'PinnedPlugins' );

PinnedPlugins.Slot = ( props ) => (
<Slot { ...props }>
{ ( fills ) => ! isEmpty( fills ) && (
<div className="edit-post-pinned-plugins">
{ fills }
</div>
) }
</Slot>
);

export default PinnedPlugins;
7 changes: 7 additions & 0 deletions edit-post/components/header/pinned-plugins/style.scss
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
.edit-post-pinned-plugins {
display: flex;

.components-icon-button {
margin-left: 4px;
}
}
19 changes: 12 additions & 7 deletions edit-post/components/header/plugin-sidebar-more-menu-item/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import { withPluginContext } from '@wordpress/plugins';
*/
import PluginsMoreMenuGroup from '../plugins-more-menu-group';

const PluginSidebarMoreMenuItem = ( { children, isSelected, icon, onClick } ) => (
const PluginSidebarMoreMenuItem = ( { children, icon, isSelected, onClick } ) => (
<PluginsMoreMenuGroup>
{ ( fillProps ) => (
<MenuItem
Expand All @@ -26,14 +26,19 @@ const PluginSidebarMoreMenuItem = ( { children, isSelected, icon, onClick } ) =>
);

export default compose(
withPluginContext,
withSelect( ( select, ownProps ) => {
const { pluginContext, target } = ownProps;
const sidebarName = `${ pluginContext.name }/${ target }`;
withPluginContext( ( context, ownProps ) => {
return {
icon: ownProps.icon || context.icon,
sidebarName: `${ context.name }/${ ownProps.target }`,
};
} ),
withSelect( ( select, { sidebarName } ) => {
const {
getActiveGeneralSidebarName,
} = select( 'core/edit-post' );

return {
isSelected: select( 'core/edit-post' ).getActiveGeneralSidebarName() === sidebarName,
sidebarName,
isSelected: getActiveGeneralSidebarName() === sidebarName,
};
} ),
withDispatch( ( dispatch, { isSelected, sidebarName } ) => {
Expand Down
107 changes: 92 additions & 15 deletions edit-post/components/sidebar/plugin-sidebar/index.js
Original file line number Diff line number Diff line change
@@ -1,37 +1,114 @@
/**
* WordPress dependencies
*/
import { Panel } from '@wordpress/components';
import { IconButton, Panel } from '@wordpress/components';
import { withDispatch, withSelect } from '@wordpress/data';
import { compose, Fragment } from '@wordpress/element';
import { __ } from '@wordpress/i18n';
import { withPluginContext } from '@wordpress/plugins';

/**
* Internal dependencies
*/
import PinnedPlugins from '../../header/pinned-plugins';
import Sidebar from '../';
import SidebarHeader from '../sidebar-header';

/**
* Renders the plugin sidebar component.
*
* @param {Object} props Element props.
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Blank comment line between @param and @return.

*
* @return {WPElement} Plugin sidebar component.
*/
function PluginSidebar( { children, name, pluginContext, title } ) {
function PluginSidebar( props ) {
const {
children,
icon,
isActive,
isPinnable = true,
isPinned,
sidebarName,
title,
togglePin,
toggleSidebar,
} = props;

return (
<Sidebar
name={ `${ pluginContext.name }/${ name }` }
label={ __( 'Editor plugins' ) }
>
<SidebarHeader
closeLabel={ __( 'Close plugin' ) }
<Fragment>
{ isPinnable && (
<PinnedPlugins>
{ isPinned && <IconButton
icon={ icon }
label={ title }
onClick={ toggleSidebar }
isToggled={ isActive }
aria-expanded={ isActive }
Copy link
Member

Choose a reason for hiding this comment

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

irrelevant: Should Button handle setting aria-expanded iff isToggled is set?

Copy link
Member Author

@gziolo gziolo May 9, 2018

Choose a reason for hiding this comment

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

Good idea, I will double check all occurrences and update IconButton and if it is applicable I will fix in a follow-up PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

By https://www.w3.org/WAI/GL/wiki/Using_aria-expanded_to_indicate_the_state_of_a_collapsible_element:

When authors use collapsible content, for example, to hide navigation menus or lists of content, the triggering link or button should indicate to screen reader users whether the collapsable content below is in the expanded or in the collapsed state. The aria-expanded attribute is used for this purpose.

It looks like it isn't always the case that both those props should exist. See:
https://github.com/WordPress/gutenberg/blob/master/components/date-time/time.js#L145-L158
In this case, buttons can be toggled. However, they don't control any collapsible content so aria-expanded is not used.

Copy link
Member

Choose a reason for hiding this comment

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

Makes sense. Thanks for looking into it.

/> }
</PinnedPlugins>
) }
<Sidebar
name={ sidebarName }
label={ __( 'Editor plugins' ) }
>
<strong>{ title }</strong>
</SidebarHeader>
<Panel>
{ children }
</Panel>
</Sidebar>
<SidebarHeader
closeLabel={ __( 'Close plugin' ) }
>
<strong>{ title }</strong>
{ isPinnable && (
<IconButton
icon={ isPinned ? 'star-filled' : 'star-empty' }
label={ isPinned ? __( 'Unpin from toolbar' ) : __( 'Pin to toolbar' ) }
onClick={ togglePin }
isToggled={ isPinned }
aria-expanded={ isPinned }
/>
) }
</SidebarHeader>
<Panel>
{ children }
</Panel>
</Sidebar>
</Fragment>
);
}

export default withPluginContext( PluginSidebar );
export default compose(
withPluginContext( ( context, ownProps ) => {
return {
icon: ownProps.icon || context.icon,
sidebarName: `${ context.name }/${ ownProps.name }`,
};
} ),
withSelect( ( select, { sidebarName } ) => {
const {
getActiveGeneralSidebarName,
isPluginItemPinned,
} = select( 'core/edit-post' );

return {
isActive: getActiveGeneralSidebarName() === sidebarName,
isPinned: isPluginItemPinned( sidebarName ),
};
} ),
withDispatch( ( dispatch, { isActive, sidebarName } ) => {
const {
closeGeneralSidebar,
openGeneralSidebar,
togglePinnedPluginItem,
} = dispatch( 'core/edit-post' );

return {
togglePin() {
togglePinnedPluginItem( sidebarName );
},
toggleSidebar() {
if ( isActive ) {
closeGeneralSidebar();
} else {
openGeneralSidebar( sidebarName );
}
},
};
} ),
)( PluginSidebar );
4 changes: 4 additions & 0 deletions edit-post/components/sidebar/sidebar-header/style.scss
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,10 @@
display: none;
margin-left: auto;

~ .components-icon-button {
margin-left: 0;
Copy link
Member

Choose a reason for hiding this comment

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

Do we really want no margin between the plugin buttons?

image

Copy link
Member Author

Choose a reason for hiding this comment

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

Added 4px margin after recommendation from @jasmussen:

screen shot 2018-05-14 at 16 09 49
screen shot 2018-05-14 at 16 10 23

}

@include break-medium() {
display: flex;
}
Expand Down
16 changes: 15 additions & 1 deletion edit-post/store/actions.js
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ export function toggleGeneralSidebarEditorPanel( panel ) {
/**
* Returns an action object used to toggle a feature flag.
*
* @param {string} feature Featurre name.
* @param {string} feature Feature name.
*
* @return {Object} Action object.
*/
Expand All @@ -91,6 +91,20 @@ export function switchEditorMode( mode ) {
};
}

/**
* Returns an action object used to toggle a plugin name flag.
*
* @param {string} pluginName Plugin name.
*
* @return {Object} Action object.
*/
export function togglePinnedPluginItem( pluginName ) {
return {
type: 'TOGGLE_PINNED_PLUGIN_ITEM',
pluginName,
};
}

/**
* Returns an action object used to check the state of meta boxes at a location.
*
Expand Down
1 change: 1 addition & 0 deletions edit-post/store/defaults.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,4 +5,5 @@ export const PREFERENCES_DEFAULTS = {
features: {
fixedToolbar: false,
},
pinnedPluginItems: {},
};
14 changes: 14 additions & 0 deletions edit-post/store/reducer.js
Original file line number Diff line number Diff line change
@@ -1,3 +1,8 @@
/**
* External dependencies
*/
import { get } from 'lodash';

/**
* WordPress dependencies
*/
Expand Down Expand Up @@ -63,6 +68,15 @@ export const preferences = combineReducers( {

return state;
},
pinnedPluginItems( state = PREFERENCES_DEFAULTS.pinnedPluginItems, action ) {
if ( action.type === 'TOGGLE_PINNED_PLUGIN_ITEM' ) {
return {
...state,
[ action.pluginName ]: ! get( state, [ action.pluginName ], true ),
};
}
return state;
},
} );

export function panel( state = 'document', action ) {
Expand Down
17 changes: 16 additions & 1 deletion edit-post/store/selectors.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
* External dependencies
*/
import createSelector from 'rememo';
import { includes, some } from 'lodash';
import { get, includes, some } from 'lodash';

/**
* Returns the current editing mode.
Expand Down Expand Up @@ -108,6 +108,21 @@ export function isFeatureActive( state, feature ) {
return !! state.preferences.features[ feature ];
}

/**
* Returns true if the the plugin item is pinned to the header.
* When the value is not set it defaults to true.
*
* @param {Object} state Global application state.
* @param {string} pluginName Plugin item name.
Copy link
Member

Choose a reason for hiding this comment

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

Similar nit: Documentation standards recommend @param and @return to be separate groups, no need to align between the two.

https://make.wordpress.org/core/handbook/best-practices/inline-documentation-standards/javascript/#functions

*
* @return {boolean} Whether the plugin item is pinned.
*/
export function isPluginItemPinned( state, pluginName ) {
const pinnedPluginItems = getPreference( state, 'pinnedPluginItems', {} );

return get( pinnedPluginItems, [ pluginName ], true );
}

/**
* Returns the state of legacy meta boxes.
*
Expand Down
Loading