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

Issue registering a PluginDocumentSettingPanel plugin #22049

Closed
vonloxx opened this issue May 2, 2020 · 7 comments · Fixed by #22763
Closed

Issue registering a PluginDocumentSettingPanel plugin #22049

vonloxx opened this issue May 2, 2020 · 7 comments · Fixed by #22763
Labels
[Feature] Extensibility The ability to extend blocks or the editing experience Needs Testing Needs further testing to be confirmed.

Comments

@vonloxx
Copy link
Contributor

vonloxx commented May 2, 2020

I was having trouble to set the settings panel to opened when registering a plugin. Here's the example code:

  const NodeDocumentSettings = () => {
    return (
      <PluginDocumentSettingPanel
        className="node-document-settings"
        title="Node"
        opened={true}
      >
        <div>Node settings</div>
      </PluginDocumentSettingPanel>
    );
  };

  registerPlugin('node-document-settings', {
    render: NodeDocumentSettings,
  });

As you see on the code, I tried to use the opened prop but to no avail.
So I tried to dispatch toggleEditorPanelOpened('node-document-settings') but that also didn't work.
I found out that the plugin is registered at local storage like this:
node-document-settings/undefined
So using toggleEditorPanelOpened('node-document-settings/undefined') worked.

Checked the code at https://github.com/WordPress/gutenberg/blob/v8.0.0/packages/edit-post/src/components/sidebar/plugin-document-setting-panel/index.js#L109
and it seems the component sets the plugin name joining the context name and props name.
I don't think it makes sense to "join" both plugin context name and prop name at the PluginDocumentSettingPanel component. Maybe it was supposed to have panelName: context.name, ?

This is happening using Drupal Gutenberg with Gutenberg v8.0.0 but I believe it can be reproduced on WordPress.

@vonloxx
Copy link
Contributor Author

vonloxx commented May 3, 2020

Or maybe all this is just a matter of stating in the documentation on how to use the dispatch toggleEditorPanelOpened()? 🤷‍♂️

@gziolo gziolo added [Feature] Extensibility The ability to extend blocks or the editing experience Needs Testing Needs further testing to be confirmed. labels May 3, 2020
@ryanwelcher
Copy link
Contributor

ryanwelcher commented May 29, 2020

I can confirm this as well in latest WordPress.

In the console, I had to pass {plugin-slug}/{panel-name} to both toggleEditorPanelToggle and toggleEditorPanelEnabled

@ryanwelcher
Copy link
Contributor

@gziolo I've added a first pass PR for this - #22763

@gziolo
Copy link
Member

gziolo commented Jun 1, 2020

Isn't the issue caused by the lack of name prop there?

props.name [string]: The machine-friendly name for the panel.

Isn't it mandatory? If it is, we can use @wordpress/warning package to warn on the JS console in development mode.

const NodeDocumentSettings = () => {
    return (
      <PluginDocumentSettingPanel
-       className="node-document-settings"
+       name="node-document-settings"
        title="Node"
        opened={true}
      >
        <div>Node settings</div>
      </PluginDocumentSettingPanel>
    );
  };

  registerPlugin('node-document-settings', {
    render: NodeDocumentSettings,
  });

@ryanwelcher
Copy link
Contributor

ryanwelcher commented Jun 1, 2020

Isn't the issue caused by the lack of name prop there?

This is part of the issue, yes. If name is undefined the panelName is set to undefined.

The other part is that the way the panelName is registered. It is created a combination of plugin_name/name_prop, which make is unclear how to access the panel programmatically as all other panels are accessed only via the name prop.

In your example above the panelName would be node-document-settings/node-document-settings and if the name was undefined, node-document-settings/undefined.

@ryanwelcher
Copy link
Contributor

I've been thinking about this more and I think that we should keep the panel naming convention as it is now. This will effectively namespace any custom panels with the name of the plugin and avoid potential naming conflicts.

As such, I think the solution should be to add a warning if the name prop is missing as @gziolo suggested and to add some documentation around calling theme custom panels. I'll update my PR accordingly.

@ryanwelcher
Copy link
Contributor

PR Updated.

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 Needs Testing Needs further testing to be confirmed.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants