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

Conversation

Projects
6 participants
@gziolo
Copy link
Member

gziolo commented Apr 26, 2018

Description

This PR adds pinning support for sidebar plugins as described by @jasmussen in #4287 (comment):

screen shot 2018-04-18 at 14 19 18

☝️ this features a "pin" icon, allowing you to pin or unpin this. It's quick and dirty, would like to polish it more, but it conveys the point.

To recap the ideas for this kind of extensibility right now, they are:

  • Every plugin that extends the editor gets a menu item in the more menu. Invoking this item invokes the plugins primary action.
  • Plugin actions could be to open a sidebar, or to open a screen takeover, or to run an action (such as spell check) directly.
  • Plugins can register a pinned extension icon next to the cog but before the more menu. These are plain svg icons that are recolored to be solid gray.
  • A user can unpin any extension icon that a plugin has pinned. The user can always use an unpinned plugin because the action will always be available in the more menu.
  • The way to unpin (or pin) an extension icon is currently the weakest aspect of the mockups, but right now they have you tap a star to toggle pinning, hollow for unpinned, solid for pinned.

and also in #3330 (comment):

Here are some mockups for how pinning extensions to the toolbar could work. This is inspired by Chrome and Firefox.

  1. You open it from the ellipsis, where all editor extensions register themselves

wolframalpha open from ellipsis

  1. This immediately opens a sidebar, because this is a "editor sidebar extension":

wolframalpha sidebar open

  1. All sidebar extensions have a "Star" icon in their heading.

wolframalpha pin to toolbar

  1. Click it to pin the icon to the toolbar:

wolframalpha extension pinned

  1. Even if you close the sidebar, the extension icon still sits there for quick access:

pinned

Repeat these steps in reverse to unpin it.

How has this been tested?

It was tested manually using a test plugin. The easiest way to apply the plugin is to copy ES5 version from this gist: https://gist.github.com/gziolo/e2954aa83aa1f823b2b05ca1660c2223.

Screenshots

Version 1 - always present 2 - unpinned by default 3 - pinned by default

pinned plugins

Types of changes

New feature (non-breaking change which adds functionality).

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.

@gziolo gziolo self-assigned this Apr 26, 2018

@gziolo gziolo requested a review from jasmussen Apr 26, 2018

@gziolo gziolo added this to To do in Extensibility via automation Apr 26, 2018

@gziolo gziolo moved this from To do to In progress in Extensibility Apr 26, 2018

aria-expanded={ isEditorSidebarOpened }
/>
<MoreMenu key="more-menu" />
<PinnedPlugins>

This comment has been minimized.

@gziolo

gziolo Apr 26, 2018

Member

Question is if we should also convert Document Settings into a plugin. I put this fill in here as a temporary solution.

This comment has been minimized.

@gziolo

gziolo May 7, 2018

Member

Out of scope for this PR.

@paulwilde

This comment has been minimized.

Copy link
Contributor

paulwilde commented Apr 26, 2018

It would be nice if the end-user had the ability to remove these pins, as it would become quite cluttered (especially on mobile) after installing a few plugins. Almost to the point of being reminiscent of Internet Explorer toolbars that you cannot get rid of.

Personally I'd prefer them to be opt-in and not forced by default.

@jasmussen

This comment has been minimized.

Copy link
Contributor

jasmussen commented Apr 26, 2018

Yep, this has been an ongoing topic for a while, and there are more details on the native extensibility ticket.

The current plan is opt out. That is, a plugin can choose to pin an icon as soon as the plugin is activated. The user can then opt out of that pinned icon by clicking it and removing the star.

Every plugin has a menu item that opens the same as the pinned icon does.

A previous plan had this be opt in, as in you had to open the plugin from the more menu first, and then explicitly pin it by checking the star.

@jasmussen

This comment has been minimized.

Copy link
Contributor

jasmussen commented Apr 26, 2018

Personally I'd prefer them to be opt-in and not forced by default.

Worth explicitly stating, in case there's any doubt: a plugin doesn't have to register a pinned icon.

In that way this is similar to top level admin menus.

@gziolo gziolo added this to the 2.9 milestone May 7, 2018

@gziolo gziolo force-pushed the add/sidebar-pinning branch from 80b02c8 to 8dc4fa2 May 7, 2018

@gziolo gziolo requested a review from karmatosed May 7, 2018

@gziolo gziolo changed the title [WIP] Plugins: Add pinning support for sidebar plugins Plugins: Add pinning support for sidebar plugins May 7, 2018

@gziolo

This comment has been minimized.

Copy link
Member

gziolo commented May 7, 2018

I have an initial implementation ready to review.

Worth explicitly stating, in case there's any doubt: a plugin doesn't have to register a pinned icon.

Every plugin item (sidebar, takeover screen, etc) - can opt out from being pinnable by using pinnable={ false } prop when composing plugin's components.

The current plan is opt out. That is, a plugin can choose to pin an icon as soon as the plugin is activated. The user can then opt out of that pinned icon by clicking it and removing the star.

I started with the easier implementation or more straightforward - everything is unpinned by default. Once the user pins (and unpins afterward), this state is persisted to be used during the next visits.

A previous plan had this be opt in, as in you had to open the plugin from the more menu first, and then explicitly pin it by checking the star.

I will look into it tomorrow. However, I need to find a way how to prevent displaying the icon for the plugin item that opted out from being pinnable.

@gziolo gziolo requested a review from WordPress/gutenberg-core May 7, 2018

@noisysocks

This comment has been minimized.

Copy link
Member

noisysocks commented May 7, 2018

Code-wise this looks great 👍

The only bug I'm noticing is that a pinned item can become stuck if the plugin is updated:

  1. Pin the custom sidebar
  2. Change the custom sidebar to have pinnable={ false }
  3. Refresh the page. The custom sidebar is still pinned and you cannot unpin it
@jasmussen

This comment has been minimized.

Copy link
Contributor

jasmussen commented May 8, 2018

Not entirely sure how to test this yet, but from your process and the screenshots, this looks right on the money. Thanks for working on this.

I think it's important that we try and get this in as soon as we possibly can — not to add stress :) — it would be good to start testing the current plan which is to allow extensions to pin themselves by default and requiring the user to unpin them if they would prefer them in the More Menu only.

@gziolo

This comment has been minimized.

Copy link
Member

gziolo commented May 8, 2018

I think it's important that we try and get this in as soon as we possibly can — not to add stress :) — it would be good to start testing the current plan which is to allow extensions to pin themselves by default and requiring the user to unpin them if they would prefer them in the More Menu only.

Taking into account what @noisysocks found out, I think I need to refactor code in a way which will allow to make the proposed flow to be the default one. I have a hunch that it can be achieved by moving Fill for pins next to the sidebar rather than having it bundled together with the menu item. Initially, it seemed to be an easier solution because all the logic for both the menu item and pin are the same.

Not entirely sure how to test this yet, but from your process and the screenshots, this looks right on the money. Thanks for working on this.

The easiest way to test it is to copy this file and paste it in the JS console. It will enable the sidebar. I also added a second variation with the palmtree icon to test that pins are always ordered the same way. When you refresh the page, you will have to paste this code again and pins should appear back even though you won't see them on the initial load.

@jasmussen

This comment has been minimized.

Copy link
Contributor

jasmussen commented May 8, 2018

Awesome, thank you for the code. Yes absolutely address Roberts' comments, didn't mean to add stress. I'm just excited to see this, as it's like a bow on our extensibility efforts.

@gziolo gziolo force-pushed the add/sidebar-pinning branch 2 times, most recently from 4427b83 to fef693b May 8, 2018

@gziolo

This comment has been minimized.

Copy link
Member

gziolo commented May 8, 2018

I think I have it all sorted out:

pinned plugins

@gziolo

This comment has been minimized.

Copy link
Member

gziolo commented May 8, 2018

The only bug I'm noticing is that a pinned item can become stuck if the plugin is updated:

  1. Pin the custom sidebar
  2. Change the custom sidebar to have pinnable={ false }
  3. Refresh the page. The custom sidebar is still pinned and you cannot unpin it

@noisysocks - it should work properly with the recent changes introduced.


- Type: `String` | `Element`
- Required: No
- Default: `admin-plugins`

This comment has been minimized.

@gziolo

gziolo May 8, 2018

Member

@jasmussen - should I pick something else as default? I assumed the more generic icon, the higher probability it gets replaced by the plugin author :)

@noisysocks
Copy link
Member

noisysocks left a comment

Code looks good. Tested locally and everything seems peachy. If Joen is happy, I'm happy.

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

PinnedPlugins.Slot = ( { fillProps } ) => (
<Slot fillProps={ fillProps }>

This comment has been minimized.

@noisysocks

noisysocks May 9, 2018

Member

minor: We can make this more future-proof by passing every prop to <Slot>. Then, if the API for Slot ever changes, PinnedPlugins.Slot will change with it.

PinnedPlugins.Slot = ( props ) => (
    <Slot { ...props }>
icon = 'admin-plugins',
isActive,
isPinned,
pinnable = true,

This comment has been minimized.

@noisysocks

noisysocks May 9, 2018

Member

minor: Would isPinnable be more consistent with e.g. isActive and isPinned?

This comment has been minimized.

@gziolo

gziolo May 9, 2018

Member

Yes, makes sense.

label={ title }
onClick={ toggleSidebar }
isToggled={ isActive }
aria-expanded={ isActive }

This comment has been minimized.

@noisysocks

noisysocks May 9, 2018

Member

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

This comment has been minimized.

@gziolo

gziolo May 9, 2018

Member

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

This comment has been minimized.

@gziolo

gziolo May 9, 2018

Member

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.

This comment has been minimized.

@noisysocks

noisysocks May 9, 2018

Member

Makes sense. Thanks for looking into it.

@gziolo gziolo force-pushed the add/sidebar-pinning branch from fef693b to 2c4b4c7 May 9, 2018

- Required: No
- Default: `true`

##### icon

This comment has been minimized.

@aduth

aduth May 10, 2018

Member

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

This comment has been minimized.

@gziolo

gziolo May 11, 2018

Member

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

This comment has been minimized.

@aduth

aduth May 11, 2018

Member

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.

This comment has been minimized.

@gziolo

gziolo May 11, 2018

Member

Yes, I like it. It should work. I’ll update PR on Monday 👍

This comment has been minimized.

@gziolo

gziolo May 13, 2018

Member

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.

if ( action.type === 'TOGGLE_PINNED_PLUGIN_ITEM' ) {
return {
...state,
[ action.pluginName ]: isUndefined( state[ action.pluginName ] ) ?

This comment has been minimized.

@aduth

aduth May 10, 2018

Member

Should we want to use Object#hasOwnProperty here so we're not dealing with oddities with my aptly-named "constructor" plugin?

$ n_
n_ > var state = {};
undefined
n_ > var action = { pluginName: 'constructor' };
undefined
n_ > _.isUndefined( state[ action.pluginName ] );
false
const defaultValue = true;
const pinnedPluginItems = getPreference( state, 'pinnedPluginItems', {} );

return isUndefined( pinnedPluginItems[ pluginName ] ) ?

This comment has been minimized.

@aduth

aduth May 10, 2018

Member

Maybe leverage get and it's defaultValue argument?

return get( getPreference( state, 'pinnedPluginItems', {} ), [ pluginName ], true );

https://lodash.com/docs/4.17.10#get


return isUndefined( pinnedPluginItems[ pluginName ] ) ?
defaultValue :
Boolean( pinnedPluginItems[ pluginName ] );

This comment has been minimized.

@aduth

aduth May 10, 2018

Member

When are we expecting the value to be anything other than a boolean here to warrant the coercion?

This comment has been minimized.

@gziolo

gziolo May 11, 2018

Member

I don’t think so.

I also agree that get from lodash will be easier to read in both places. I forgot about the fact that it checks if value is defined 😎

@gziolo gziolo force-pushed the add/sidebar-pinning branch from 2c4b4c7 to 86cb16b May 13, 2018

@gziolo

This comment has been minimized.

Copy link
Member

gziolo commented May 13, 2018

@aduth, I addressed your feedback, should be ready for another check.

@jasmussen

This comment has been minimized.

Copy link
Contributor

jasmussen commented May 14, 2018

I think this would be good to get in sooner rather than later. 👍 👍

@gziolo gziolo force-pushed the add/sidebar-pinning branch from 86cb16b to 7a31494 May 14, 2018

@gziolo

This comment has been minimized.

Copy link
Member

gziolo commented May 14, 2018

Fixed a regression introduced in Document & Block sidebar with 7a31494.

@aduth

aduth approved these changes May 14, 2018

Copy link
Member

aduth left a comment

Works great 👍

import Sidebar from '../';
import SidebarHeader from '../sidebar-header';

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

This comment has been minimized.

@aduth

aduth May 14, 2018

Member

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

@@ -22,6 +22,10 @@
display: none;
margin-left: auto;

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

This comment has been minimized.

@aduth

aduth May 14, 2018

Member

Do we really want no margin between the plugin buttons?

image

This comment has been minimized.

@gziolo

gziolo May 14, 2018

Member

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

* When the value is not set it defaults to true.
*
* @param {Object} state Global application state.
* @param {string} pluginName Plugin item name.

This comment has been minimized.

@aduth

aduth May 14, 2018

Member

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

@@ -23,6 +23,7 @@ const plugins = {};
* @param {string} name The name of the plugin.
* @param {Object} settings The settings for this plugin.
* @param {Function} settings.render The function that renders the plugin.
* @param {string} settings.icon An icon to be shown in the UI.

This comment has been minimized.

@aduth

aduth May 14, 2018

Member

Can be element as well?

Edit post: Add margin between pinned icons
Includes also JSDoc updates.

@gziolo gziolo merged commit f071482 into master May 14, 2018

2 checks passed

codecov/project 44.12% (-0.12%) compared to 09e2da3
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

Extensibility automation moved this from In progress to Done May 14, 2018

@gziolo gziolo deleted the add/sidebar-pinning branch May 14, 2018

@mtias

This comment has been minimized.

Copy link
Contributor

mtias commented May 18, 2018

This a great final touch for the Plugins API. Great work @gziolo !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment