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

PluginPrePublishPanel uses icon property and inherits from registerPlugin #16378

Conversation

@ryanwelcher
Copy link
Contributor

commented Jul 1, 2019

Description

This PR adds the icon parameter to the PluginPrePublishPanel SlotFill and allows the icon to be inherited via the registerPlugin command.

How has this been tested?

Screenshots

pre-publish-panel

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.

@ryanwelcher ryanwelcher requested review from talldan and youknowriad as code owners Jul 1, 2019

@ryanwelcher

This comment has been minimized.

Copy link
Contributor Author

commented Jul 1, 2019

Related #13361

@ryanwelcher ryanwelcher changed the title Try/plugin pre publish panel inherit icon PluginPrePublishPanel uses icon property. Jul 1, 2019

@ryanwelcher ryanwelcher changed the title PluginPrePublishPanel uses icon property. PluginPrePublishPanel uses icon property and inherits from registerPlugin Jul 2, 2019

@youknowriad youknowriad requested review from gziolo, mtias and jasmussen Jul 2, 2019

@ryanwelcher

This comment has been minimized.

Copy link
Contributor Author

commented Aug 26, 2019

@youknowriad @sarahmonster Wondering how I can help move this forward?

@gziolo

This comment has been minimized.

Copy link
Member

commented Aug 26, 2019

From a technical perspective, this looks good. It needs some testing and approval from the design team.

@gziolo

This comment has been minimized.

Copy link
Member

commented Aug 30, 2019

Can we apply the same set of changes to the post publish panel for consistency?

Screen Shot 2019-08-30 at 14 36 55

I tested with the following code:

( function() {
	var el = wp.element.createElement;
	var Fragment = wp.element.Fragment;
	var __ = wp.i18n.__;
	var registerPlugin = wp.plugins.registerPlugin;
	var PluginPostPublishPanel = wp.editPost.PluginPostPublishPanel;
	var PluginPrePublishPanel = wp.editPost.PluginPrePublishPanel;

	function PanelContent() {
		return el(
			'p',
			{},
			__( 'Here is the panel content!' )
		);
	}

	function MyPublishPanelPlugin() {
		return el(
			Fragment,
			{},
			el(
				PluginPrePublishPanel,
				{
					className: 'my-publish-panel-plugin__pre',
					title: __( 'My pre publish panel' )
				},
				el(
					PanelContent,
					{}
				)
			),
			el(
				PluginPostPublishPanel,
				{
					className: 'my-publish-panel-plugin__post',
					title: __( 'My post publish panel' )
				},
				el(
					PanelContent,
					{}
				)
			)
		);
	}

	registerPlugin( 'my-publish-panel-plugin', {
		icon: 'palmtree',
		render: MyPublishPanelPlugin
	} );
} )();

As noted earlier, for the pre-publish panel it's ready to go.

@ryanwelcher

This comment has been minimized.

Copy link
Contributor Author

commented Aug 31, 2019

@gziolo Thanks for the reivew on this!
I did a separate PR for the post publish panel here - #16383

@ryanwelcher

This comment has been minimized.

Copy link
Contributor Author

commented Aug 31, 2019

Icons were discussed a bit in #14180 as well, where we came to the conclusion that we'd allow them, but only in grayscale format.

@sarahmonster I believe that icons that are used in the Document Sidebar are forced to greyscale. At least when using the PluginDocumentSettingPanel SlotFill. Can we do the same here?

@gziolo
gziolo approved these changes Sep 2, 2019
Copy link
Member

left a comment

Seeing #16383, let's move forward with this one as soon as there is from the design team.

@gziolo gziolo requested review from kjellr and mapk Sep 4, 2019

@kjellr

This comment has been minimized.

Copy link
Contributor

commented Sep 4, 2019

I believe that icons that are used in the Document Sidebar are forced to greyscale. At least when using the PluginDocumentSettingPanel SlotFill. Can we do the same here?

Yeah, assuming that someone's using the standard method inside of a Panel, the icons will be grayscale due to this rule:

filter: grayscale(1) !important;

As long as that's all set, this seems like a solid improvement to me — as noted above, it does help make clear which panels are coming from a plugin.

@ryanwelcher ryanwelcher force-pushed the ryanwelcher:try/plugin-pre-publish-panel-inherit-icon branch from 29c2ef2 to 437bd99 Sep 4, 2019

@mapk
mapk approved these changes Sep 4, 2019
Copy link
Contributor

left a comment

Yep! I agree with the comments here. The icons help clarify which panels are from specific plugins. A good improvement. LGTM.

@ryanwelcher

This comment has been minimized.

Copy link
Contributor Author

commented Sep 6, 2019

Wanted to confirm that custom SVGs are rendered in greyscale.
Edit_Post_‹gutenberg-dev_test—_WordPress

@ryanwelcher

This comment has been minimized.

Copy link
Contributor Author

commented Sep 6, 2019

@gziolo I've resolved the failures on this PR. I believe that this and #16383 are both ready for merge.

@ryanwelcher ryanwelcher merged commit 0f59a72 into WordPress:master Sep 6, 2019

2 checks passed

pull-request-automation
Details
Travis CI - Pull Request Build Passed
Details

@ryanwelcher ryanwelcher deleted the ryanwelcher:try/plugin-pre-publish-panel-inherit-icon branch Sep 6, 2019

@youknowriad youknowriad added this to the Gutenberg 6.5 milestone Sep 14, 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.