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

Add initialOpen support for PluginDocumentSettingPanel component. #18985

Conversation

Addison-Stavlo
Copy link
Contributor

@Addison-Stavlo Addison-Stavlo commented Dec 6, 2019

Description

Added an optional initialOpen prop to the PluginDocumentSettingPanel component.

This component is defined using the PanelBody as a child. Since the PanelBody already has support for the initialOpen prop, we are just adding support for that prop to be passed in from the PluginDocumentSettingPanel parent.

When initialOpen is defined and passed a truthy value, it will always start open and will ignore its opened and onToggle properties from the data store connection. Otherwise, behavior remains as it was prior to this change.

Why?

Last week I found the case where we wanted to have a PluginDocumentSettingPanel always open by default. In digging I found that the PanelBody had support for an initialOpen property but that the PluginDocumentSettingPanel was not set up to pass this prop down to the PanelBody. This was disappointing as it would have made things very simple, so I figured I would update it and see what others think about adding this optional functionality.

Without this our fix had to be a temporary subscription to the data store. A function to check if the state was closed and to dispatch the toggle runs once and is then unsubscribed. While this is a fairly simple thing to do, adding support for initialOpen will allow this component to be more robust and more easily usable for other cases in the future.

How has this been tested?

  • Run this PR.
  • Add a PluginDocumentSettingPanel(I added this in an active plugin).
  • Test its behavior without adding the initialOpen prop, verify there are no changes in functionality.
  • Add the initialOpen prop and give it a falsey value. Verify again there are no changes in functionality.
  • Give the initialOpen a truthy value. Verify that the panel always starts open when returning to the editor regardless of its last state.

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.
  • I've updated all React Native files affected by any refactorings/renamings in this PR. .

@noahtallen noahtallen added [Package] Edit Post /packages/edit-post [Type] Enhancement A suggestion for improvement. labels Dec 17, 2019
Copy link
Member

@noahtallen noahtallen left a comment

Choose a reason for hiding this comment

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

Overall, this change looks pretty good to me :)

opened={ opened }
onToggle={ onToggle }
initialOpen={ initialOpen }
opened={ initialOpen ? undefined : opened }
Copy link
Member

Choose a reason for hiding this comment

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

This feels a little weird for me -- why do we need to set it to undefined if the initialOpen prop exists?

Copy link
Contributor Author

@Addison-Stavlo Addison-Stavlo Dec 17, 2019

Choose a reason for hiding this comment

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

I did this because of how the interior component PanelBody is set up.

For the opened property:

const { title, children, opened, className, icon, forwardedRef } = this.props;
const isOpened = opened === undefined ? this.state.opened : opened;

If the opened prop is undefined it falls back to using its default behavior of relying on state as opposed to the store connection. So if we did not set it to undefined in this declaration, it would overwrite the behavior of initialOpen.

Similarly, for the toggle function:

toggle( event ) {
event.preventDefault();
if ( this.props.opened === undefined ) {
this.setState( ( state ) => ( {
opened: ! state.opened,
} ) );
}
if ( this.props.onToggle ) {
this.props.onToggle();
}
}

If opened is undefined we rely on the components state as opposed to the store connection. And onToggle will run if it is defined.

Essentially, having these properties defined will overwrite the behavior we want from initialOpen. So if initialOpen is truthy, we need to explicitly set these as undefined so they don't overwrite its behavior.

Copy link
Member

Choose a reason for hiding this comment

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

ah interesting. Cool!

Copy link
Contributor

Choose a reason for hiding this comment

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

Unfortunately It looks like when the panel is using store state, the open/closed state is synced to local storage. This seems to circumvent that, which I don't think is a positive thing.

@talldan talldan added First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository Needs Design Feedback Needs general design feedback. labels Dec 18, 2019
@talldan
Copy link
Contributor

talldan commented Dec 18, 2019

I've added the Needs Design Feedback label so that a designer can take a look at what the desired behaviour is for plugin panels, and whether the editor should allow plugin panels to be initially opened.

@noahtallen noahtallen removed the First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository label Jan 15, 2020
@aaroncampbell
Copy link
Member

Ran into this today myself. I think the desired behavior, at least for me, would be just like PanelBody in that initialOpen would set the state if none has already been set by the user and use their their setting otherwise.

@paaljoachim
Copy link
Contributor

It would be great with a more detailed test approach for designers to test out this PR.
Thanks!

@Addison-Stavlo
Copy link
Contributor Author

Addison-Stavlo commented Nov 11, 2020

After looking back on this after quite some time, it looks like the initial goal was a bit flawed. That is, I was trying to make initialOpen work in a way in which it forces the panel to be open everytime the editor is loaded regardless of how the user had left it. This seems to go against how this prop is used in the PanelBody, which would be only to initialize the state and then use it when available.

It looks like we cannot easily add and pass the initialOpen prop down to the PanelBody here, since we are already passing opened which is designed to override initialOpen. If we do want the option to make a plugin panel support initialOpen we will need to rethink how we go about initializing the panels preference in the edit-post data store.

@Addison-Stavlo Addison-Stavlo deleted the add/plugin-document-setting-panel-initial-open branch November 11, 2020 21:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Design Feedback Needs general design feedback. [Package] Edit Post /packages/edit-post [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants