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

Extensibility: Add possibility to disable document settings panels registered by plugins #16900

Merged
merged 1 commit into from
Aug 9, 2019

Conversation

gziolo
Copy link
Member

@gziolo gziolo commented Aug 5, 2019

Description

Follow up for #13361. It addresses my own comment #13361 (review):

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 PR introduces internal SlotFill pair to offer a way for users to disable/enable registered Document settings panels by plugins.

How has this been tested?

Register a custom document setting panel in the sidebar with the following snippet:

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

	function MyDocumentSettingPlugin() {
		return el(
			PluginDocumentSettingPanel,
			{
				className: 'my-document-setting-plugin',
				title: 'My Custom Panel 1'
			},
			__( 'My Document Setting Panel 1' )
		);
	}

	registerPlugin( 'my-document-setting-plugin-1', {
		render: MyDocumentSettingPlugin
	} );
} )();

In my tests, I registered multiple panels to ensure that it's possible to enable/disable them and everything is rendered in the same order in both the sidebar for Document setting and in the Options modal.

Screenshots

Screen Shot 2019-08-05 at 09 47 05

options-modal

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.

@gziolo gziolo added [Type] Enhancement A suggestion for improvement. [Feature] Extensibility The ability to extend blocks or the editing experience labels Aug 5, 2019
@gziolo gziolo requested a review from ryanwelcher August 5, 2019 07:49
@gziolo gziolo requested a review from talldan as a code owner August 5, 2019 07:49
@gziolo gziolo requested a review from noisysocks August 5, 2019 07:49
@gziolo gziolo changed the title Extensibility: Add possibility to disable document settings panels registered plugins Extensibility: Add possibility to disable document settings panels registered by plugins Aug 5, 2019
@gziolo gziolo self-assigned this Aug 5, 2019
@gziolo gziolo force-pushed the update/modal-remove-plugin-sidebar-section branch from 8d2bbe3 to 7d2ef43 Compare August 8, 2019 09:49
Copy link
Member

@jorgefilipecosta jorgefilipecosta left a comment

Choose a reason for hiding this comment

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

This works well in my tests 👍
I noticed that if the plugin is not currently showing the panel no option to hide it appears. I imagine some plugins may only show the panel in rare circumstances, so for the user to disable the panel, it first needs to create a case where it appears. Would it make sense to allow plugins to explicitly add an option to disable their panel even if it is not being shown right now?
Another related question: If there is a plugin panel that is currently being drawn but not shows because the user disabled it, would it make sense to show a small UI feedback in the document settings indicating hidden panels exist?

@gziolo
Copy link
Member Author

gziolo commented Aug 8, 2019

I noticed that if the plugin is not currently showing the panel no option to hide it appears. I imagine some plugins may only show the panel in rare circumstances, so for the user to disable the panel, it first needs to create a case where it appears. Would it make sense to allow plugins to explicitly add an option to disable their panel even if it is not being shown right now?

Can you clarify what needs to happen to have that? Do you mean having something like the following?:

{ showMyPanel && <PluginDocumentSettingPanel /> }

This would prevent the option to be visible in the modal, yes. Any ideas about how we could approach it, I can't think of any solution which works out of the box. Otherwise, this option will always be present in the modal as it's not guarded internally by any check.

Another related question: If there is a plugin panel that is currently being drawn but not shows because the user disabled it, would it make sense to show a small UI feedback in the document settings indicating hidden panels exist?

@mtias, @mapk or @jasmussen any thoughts about that? I guess we might cover it with the new tips if that makes sense. I'm not convinced we need it though.

@mtias
Copy link
Member

mtias commented Aug 8, 2019

I'm not convinced we need it though.

Agreed.

@mapk
Copy link
Contributor

mapk commented Aug 8, 2019

When hiding blocks in the Block Manager, we didn't plan how to communicate it outward, and it's definitely bit me in the butt a few times. It was a necessary exploration though. With this example, I agree that we don't need to notify the user of which panels are turned off... right now. It's something to watch as we introduce this though.

Copy link
Member

@jorgefilipecosta jorgefilipecosta left a comment

Choose a reason for hiding this comment

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

Can you clarify what needs to happen to have that? Do you mean having something like the following?:

{ showMyPanel && }

This would prevent the option to be visible in the modal, yes. Any ideas about how we could approach it, I can't think of any solution which works out of the box. Otherwise, this option will always be present in the modal as it's not guarded internally by any check.

One option would be to expose the fill that allows disabling a specific panel so if a plugin things it is a better UI to always have the option to disable the panel available the plugin could do it.

But on second thought, this just makes our API bigger while trying to solve a problem that we don't if it exists.

I think this PR is ready 👍

@gziolo
Copy link
Member Author

gziolo commented Aug 9, 2019

But on second thought, this just makes our API bigger while trying to solve a problem that we don't if it exists.

In 99% of cases, we should offer a way to disable/enable the panel which nicely aligns with all panels included in core. So we can live with it :)

@gziolo gziolo merged commit aff692e into master Aug 9, 2019
@gziolo gziolo deleted the update/modal-remove-plugin-sidebar-section branch August 9, 2019 13:02
@gziolo gziolo added this to the Gutenberg 6.3 milestone Aug 9, 2019
@noisysocks
Copy link
Member

Should we add some docs for this? 😇

@gziolo
Copy link
Member Author

gziolo commented Aug 12, 2019

Should we add some docs for this? 😇

What kind of docs would you like to see? This is all internal to the edit-post module.

@noisysocks
Copy link
Member

Ah, apologies. I misread and thought EnablePluginDocumentSettingPanelOption was something that plugin developers had to include in their code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Extensibility The ability to extend blocks or the editing experience [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants