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

Introduces PluginSettingsSidebar slotfill to the Document sidebar. #13361

Merged
merged 62 commits into from Jul 3, 2019

Conversation

@ryanwelcher
Copy link
Contributor

commented Jan 17, 2019

Description

Introduce a new slotFill below Status & Availability panel in the Document sidebar.
Closes #13357

How has this been tested?

This has been testing locally and there are tests on the PR.

Screenshots

slotfill

Types of changes

New feature that only affects the settings-sidebar component.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.

Basic Usage Example:

const { registerPlugin } = wp.plugins;
const { PluginDocumentSetting } = wp.editPost;

const MyDocumentSettingTest = () => (
	<PluginDocumentSetting className="my-document-setting-plugin">
		<p>My Document Setting Panel</p>
	</PluginDocumentSetting>
);

registerPlugin( 'document-setting-test', { render: MyDocumentSettingTest } );

@ryanwelcher ryanwelcher changed the title Feature/settings panel slot Introduce PluginSettingsSidebar slotfill to the Document sidebar. Jan 17, 2019

@ryanwelcher ryanwelcher changed the title Introduce PluginSettingsSidebar slotfill to the Document sidebar. Introduces PluginSettingsSidebar slotfill to the Document sidebar. Jan 17, 2019

@adamsilverstein
Copy link
Contributor

left a comment

LGTM! This is an elegant solution to a useful extension point.

@youknowriad
Copy link
Contributor

left a comment

Referring to the related slack discussion. https://wordpress.slack.com/archives/C02QB2JS7/p1545318632348900

Let's wait a bit before introducing this as Phase 2 work could have an impact on backward compatibilty here.

@gziolo

This comment has been minimized.

Copy link
Member

commented Jan 21, 2019

Noting, that there is also a tutorial which explains how to create a sidebar for your plugin - - it would be an alternative supported as of today:
https://github.com/WordPress/gutenberg/blob/master/docs/designers-developers/developers/tutorials/sidebar-tutorial/plugin-sidebar-0.md

There was also a related discussion on Twitter on that topic during the weekend:

https://twitter.com/AlexStandiford/status/1086313778625593344

There was mention of the article by @ocean90:
https://dominikschilling.de/how-to-edit-post-meta-outside-of-a-gutenberg-block/

@gziolo

This comment has been minimized.

Copy link
Member

commented Apr 24, 2019

@youknowriad any new thoughts on this proposal? Code wise it looks solid. There is only the question related to the UI considerations and how does it fit in the wider picture of the block editor on other screens like the widget one.

@adamsilverstein adamsilverstein requested a review from youknowriad May 1, 2019

@youknowriad

This comment has been minimized.

Copy link
Contributor

commented May 6, 2019

I think we can explore this a bit further now. The original reasons for delaying this are still valid (could become an inspector control in phase 2 if we ever introduce a "Document" or "Post" block) but it feels a bit too seen and this use-case is legit.

Can we expaned the description of the PR with a plugin example to test it?

I think we should rename this to DocumentSetting as it's a more semantic name that sets the expectation that this is about adding new document settings. They are now displayed in the document panel but the framework (editor) can display them in a different way if needed.

Also, is there a way to display the plugin icon in the panel header (to clarify the origin of the panel).

@gziolo
Copy link
Member

left a comment

This looks solid and is nearly ready to go. I left some comments with nitpicks so we ensure it's properly documented.

There is also a separate issue which we need to consider in the follow-up - how to expose those registered panels in the Options modal:

Screen Shot 2019-06-17 at 12 02 08

This is challenging on its own, but we can always refactor PluginDocumentSettingFill to render two fills, where one targets the sidebar and another one would render to the Options modal :) Prior art for the pinned plugins feature:

{ isPinnable && (
<PinnedPlugins>
{ isPinned && <IconButton
icon={ icon }
label={ title }
onClick={ toggleSidebar }
isToggled={ isActive }
aria-expanded={ isActive }
/> }
</PinnedPlugins>
) }

return (
<Fill>
<PanelBody
name={ name }

This comment has been minimized.

Copy link
@gziolo

gziolo Jun 17, 2019

Member

PanelBody doesn's support name prop:

const { title, children, opened, className, icon, forwardedRef } = this.props;

I'm also wondering whether we shouldn't explicitly pass only those props which are documented. See other components for reference:


<PanelBody
className={ className }
initialOpen={ initialOpen || ! title }
title={ title }
>

Out of scope of this PR: There is also this question, whether we would like to relax those limitations in the future and say in documentation that we support also all props which PanelBody or PanelRow support. We do something similar for some of the @wordpress/components which are composed of other components.

This comment has been minimized.

Copy link
@ryanwelcher

ryanwelcher Jun 21, 2019

Author Contributor

@gziolo All good points, I removed the {...props} and only allow the props that are documented.

* @param {Object} props Component properties.
* @param {string} [props.name] The machine-friendly name for the panel.
* @param {string} [props.className] An optional class name added to the row.
* @param {string} [props.title] The title of the panel

This comment has been minimized.

Copy link
@gziolo

gziolo Jun 17, 2019

Member

We should also include icon which is now supported with the default coming from the plugin's definition. This is how it's documented for the PluginSidebar:

* @param {string|Element} [props.icon=inherits from the plugin] 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.

This comment has been minimized.

Copy link
@ryanwelcher

ryanwelcher Jun 21, 2019

Author Contributor

Updated

@@ -20,6 +20,7 @@ import PostLink from '../post-link';
import DiscussionPanel from '../discussion-panel';
import PageAttributes from '../page-attributes';
import MetaBoxes from '../../meta-boxes';
import PluginDocumentSetting from '../plugin-document-setting-panel';

This comment has been minimized.

Copy link
@gziolo

gziolo Jun 17, 2019

Member
Suggested change
import PluginDocumentSetting from '../plugin-document-setting-panel';
import PluginDocumentSettingPanel from '../plugin-document-setting-panel';

There is also a corresponding usage.

This comment has been minimized.

Copy link
@ryanwelcher

ryanwelcher Jun 21, 2019

Author Contributor

Nice catch, I've updated.

it( 'Should render a custom panel inside Document Setting sidebar', async () => {
await openDocumentSettingsSidebar();
const pluginDocumentSettingsText = await page.$eval( '.edit-post-sidebar .my-document-setting-plugin', ( el ) => el.innerText );
expect( pluginDocumentSettingsText ).toBe( 'My Document Setting Panel' );

This comment has been minimized.

Copy link
@gziolo

gziolo Jun 17, 2019

Member

How about we make it similar to Should open plugins sidebar using More Menu item and render content test and do snapshot testing as well to ensure it is properly formatted as a panel? It should still take 2 lines :)

This comment has been minimized.

Copy link
@ryanwelcher

ryanwelcher Jun 21, 2019

Author Contributor

I've made an update here but am a little unclear still - can you confirm this is what you were looking for @gziolo ?

This comment has been minimized.

Copy link
@gziolo

gziolo Jun 24, 2019

Member

E2e test fails in the current shape, I think it's fine to remove line 84 as this should be part of the snapshot as well. I thought about something like:

const pluginDocumentSettingsPanel = await page.$eval( '.edit-post-sidebar .my-document-setting-plugin', ( el ) => el.innerHTML );
expect( pluginDocumentSettingsPanel ).toMatchSnapshot();
@gziolo

This comment has been minimized.

Copy link
Member

commented Jun 24, 2019

E2e test fails with the latest changes:

https://travis-ci.com/WordPress/gutenberg/jobs/209846043#L879

FAIL  packages/e2e-tests/specs/plugins/plugins-api.test.js (19.869s)
  ● Using Plugins API › Document Setting Custom Panel › Should render a custom panel inside Document Setting sidebar
    expect(received).toBe(expected) // Object.is equality
    Expected: "My Document Setting Panel"
    Received: "
    My Custom Panel
@gziolo

This comment has been minimized.

Copy link
Member

commented Jun 24, 2019

@youknowriad, I think this PR addresses your comments, can you do sanity check? There is still e2e test which needs to be fixed which should be straightforward. We might also want to include those custom panels in the Options dialog so users could disable them the same way they can do it for all other panels.

@youknowriad

This comment has been minimized.

Copy link
Contributor

commented Jun 25, 2019

We might also want to include those custom panels in the Options dialog so users could disable them the same way they can do it for all other panels.

Yeah, this seems like something that is needed. I'm fine if it's a separate PR though. Whatever works best for you all.

@youknowriad

This comment has been minimized.

Copy link
Contributor

commented Jun 25, 2019

Tested this, it works great. A few questions:

  • Will we need an id to "enable/disable" the panel or is the plugin's identifier sufficient?
  • It does feel like we may need a way to define the position (or at least a priority or something like that) for the panel. (Also something that we can explore separately).
@ryanwelcher

This comment has been minimized.

Copy link
Contributor Author

commented Jun 25, 2019

@youknowriad I was able to remove the custom panels using removePanel and looking up the id in the markup.

I have been thinking about the priority aspect as well, I was thinking that adding it to registerPlugin() may make sense. This will give us the flexibility to add it to other SlotFills in the future and also for the existing ones such as PluginPrePublishPanel and PluginPostPublishPanel - both of which could benefit from it.

@ryanwelcher

This comment has been minimized.

Copy link
Contributor Author

commented Jul 1, 2019

@gziolo @youknowriad I've updated the e2e test and everything is passing. Is there anything else that needs to be addressed on this?

@ryanwelcher

This comment has been minimized.

Copy link
Contributor Author

commented Jul 2, 2019

@youknowriad I have created #16384 as an approach to manage priority. Any feedback is apprectiated!

@ryanwelcher ryanwelcher referenced this pull request Jul 2, 2019

Open

SlotFill sorting with a priority property #16384

5 of 5 tasks complete
@@ -88,6 +88,7 @@ export function initializeEditor( id, postType, postId, settings, initialEdits )
}

export { default as PluginBlockSettingsMenuItem } from './components/block-settings-menu/plugin-block-settings-menu-item';
export { default as PluginDocumentSettingPanel } from './components/sidebar/plugin-document-setting-panel';

This comment has been minimized.

Copy link
@youknowriad

youknowriad Jul 2, 2019

Contributor

@aduth any thougs on the naming?

This comment has been minimized.

Copy link
@aduth

aduth Jul 3, 2019

Member

Seems fine to me as proposed.

@youknowriad youknowriad merged commit 472e255 into WordPress:master Jul 3, 2019

1 check passed

Travis CI - Pull Request Build Passed
Details

@github-actions github-actions bot added this to the Gutenberg 6.1 milestone Jul 3, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.