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 disable support to PostStatus settings panel #17117

Merged
merged 5 commits into from Sep 11, 2019

Conversation

@noahtallen
Copy link
Contributor

commented Aug 21, 2019

While many other panels support the isEditorPanelEnabled option, the PostStatus panel does not. In my own project, I was trying to use dispatch('core/edit-post').removeEditorPanel( 'post-status' ); to remove this panel in a particular context. I realized that it wasn't working and found that the component did not respect the setting.

Note: I have a PR up here which is the context of finding this bug: Automattic/wp-calypso#35643

Description

  • If the 'post-status' panel is not enabled, the component now returns null. Previously, it always returned the full Panel regardless of whether it was enabled. I used the prior art of the featured-image panel to make these changes.

How has this been tested?

  1. I tested this fix with my own project and removeEditorPanel( 'post-status' ); now works correctly from a JS context. (This was using Gutenberg master on the Gutenberg docker setup.)
  2. I tried wp.data.dispatch('core/edit-post').removeEditorPanel( 'post-status' ); from the console on a normal post (outside of my project), and it works correctly. I would recommend this option in order to test this branch.

Types of changes

  • Bug fix.

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.
Copy link
Member

left a comment

Could use the ifCondition HOC here.

@youknowriad

This comment has been minimized.

Copy link
Contributor

commented Aug 21, 2019

Do we know why this has been excluded initially? Other than that, this LGTM

@noahtallen

This comment has been minimized.

Copy link
Contributor Author

commented Aug 21, 2019

I pinged the guy who originally added support and he directed me to this comment: #10215 (comment)

In particular, they didn't want to allow a user to remove the panel from a checkbox in the Options modal. This PR doesn't add a checkbox for it, so I think it should still be ok since we're only adding developer support.

ifCondition

I'll check that out! It sounds nifty. I think I saw it in use on one of the other panel components. 🤔

@youknowriad

This comment has been minimized.

Copy link
Contributor

commented Aug 21, 2019

Ok adding @mtias to the discussion because it was his decision.

@noahtallen

This comment has been minimized.

Copy link
Contributor Author

commented Aug 21, 2019

Some additional context for our situation is that we have a separate editor for header/footer. In that editor, the user can see the PostStatus panel even though the visibility/publish/trash shouldn't be changed by the end user.

Another note is that for the time being we're just hiding the panel with CSS, but I think this would be cleaner :)

@gwwar

This comment has been minimized.

Copy link
Contributor

commented Aug 21, 2019

This tests well for me, and code makes sense. Sounds like we probably just need a confirmation on if we want to allow this or not in the project.

post-status

@gwwar

This comment has been minimized.

Copy link
Contributor

commented Aug 22, 2019

@noahtallen maybe rebase to see if that clears up the tests? Might have picked up a bad snapshot of the npm version in travis.

@noahtallen

This comment has been minimized.

Copy link
Contributor Author

commented Aug 22, 2019

Need to work on my git forking skills. Will do :)

@noahtallen noahtallen force-pushed the noahtallen:patch-1 branch from 3197fb2 to e5196eb Aug 23, 2019
@noahtallen noahtallen requested a review from nerrad as a code owner Aug 23, 2019
@noahtallen noahtallen force-pushed the noahtallen:patch-1 branch from e5196eb to 9649e72 Aug 23, 2019
@noahtallen

This comment has been minimized.

Copy link
Contributor Author

commented Aug 23, 2019

Looks like the build was passing for a sec while I was rebasing, but is failing again. Everything seems to be testing ok locally, so I'm wondering if this is a flakey test 🤔I can't rerun the build myself, so I thought I'd check if this is common :)

@youknowriad

This comment has been minimized.

Copy link
Contributor

commented Aug 23, 2019

Looks like there might be an intermitent faiilure in the list block test (keyboard to indent/outdent). Pinging @gziolo @ellatrix in case they know more about it.

I restarted the job to see.

@noahtallen

This comment has been minimized.

Copy link
Contributor Author

commented Aug 23, 2019

I restarted the job to see.

Looks like it passed!

Copy link
Member

left a comment

To programmatically remove editor panels from UI you should use removeEditorPanel action together with isEditorPanelRemoved selector implemented in #11802.

isEditorPanelEnabled isn't a good fit here as it combines also the state from UI. In theory, it works as it checks both conditions behind the scenes but as explained by @mtias we don't want to allow to disable the post status panel through UI.

Once, this is updated, this PR is good to go :)

@gziolo

This comment has been minimized.

Copy link
Member

commented Aug 30, 2019

It would be great to find a good place in docs where we document how developers can programmatically remove all different panels. I opened #17281 to track it.

@noahtallen noahtallen force-pushed the noahtallen:patch-1 branch from 9649e72 to 1044f9c Sep 4, 2019
@noahtallen

This comment has been minimized.

Copy link
Contributor Author

commented Sep 4, 2019

use removeEditorPanel action together with isEditorPanelRemoved selector

Sounds great! Updated to reflect that.

@gziolo

This comment has been minimized.

Copy link
Member

commented Sep 4, 2019

Should we update the following e2e test to validate that Status & Visibility panel can be removed as well? See:

expect( await findSidebarPanelWithTitle( 'Categories' ) ).toBeDefined();
expect( await findSidebarPanelWithTitle( 'Tags' ) ).toBeDefined();
expect( await findSidebarPanelWithTitle( 'Featured Image' ) ).toBeDefined();
expect( await findSidebarPanelWithTitle( 'Excerpt' ) ).toBeDefined();
expect( await findSidebarPanelWithTitle( 'Discussion' ) ).toBeDefined();
await page.evaluate( () => {
const { removeEditorPanel } = wp.data.dispatch( 'core/edit-post' );
removeEditorPanel( 'taxonomy-panel-category' );
removeEditorPanel( 'taxonomy-panel-post_tag' );
removeEditorPanel( 'featured-image' );
removeEditorPanel( 'post-excerpt' );
removeEditorPanel( 'discussion-panel' );
} );
expect( await findSidebarPanelWithTitle( 'Categories' ) ).toBeUndefined();
expect( await findSidebarPanelWithTitle( 'Tags' ) ).toBeUndefined();
expect( await findSidebarPanelWithTitle( 'Featured Image' ) ).toBeUndefined();
expect( await findSidebarPanelWithTitle( 'Excerpt' ) ).toBeUndefined();
expect( await findSidebarPanelWithTitle( 'Discussion' ) ).toBeUndefined();

@noahtallen noahtallen requested a review from ajitbohra as a code owner Sep 4, 2019
@noahtallen noahtallen requested a review from ntwb as a code owner Sep 4, 2019
@noahtallen

This comment has been minimized.

Copy link
Contributor Author

commented Sep 4, 2019

Should we update the following e2e test

Awesome! Just updated that and added a check for 'Status & Visibility'.

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

left a comment

Nice work, thanks 👍

There is still one line missing in the test as noted in the comment

packages/e2e-tests/specs/sidebar.test.js Show resolved Hide resolved
noahtallen added 4 commits Aug 20, 2019
While many other panels support the isEditorPanelEnabled option, the
PostStatus panel does not. If the 'post-status' panel is not enabled, the
component now returns null. Previously, it always returned the full
Panel regardless of whether it was enabled.

This is cleaner than returning null, since ifCondition does that for us.
We do this since the panel should never be disabled with the enabled
selector, but it can be programmatically removed.
@noahtallen noahtallen force-pushed the noahtallen:patch-1 branch from 44373a0 to 9302f22 Sep 9, 2019
@noahtallen

This comment has been minimized.

Copy link
Contributor Author

commented Sep 9, 2019

Updated the test to handle that condition. :)

@gziolo

This comment has been minimized.

Copy link
Member

commented Sep 11, 2019

@noahtallen - thanks for addressing all feedback and congrats on your first code contribution to Gutenberg 🎉

@gziolo gziolo merged commit 9fef2e8 into WordPress:master Sep 11, 2019
2 checks passed
2 checks passed
pull-request-automation
Details
Travis CI - Pull Request Build Passed
Details
@gziolo gziolo added this to the Gutenberg 6.5 milestone Sep 11, 2019
@noahtallen noahtallen deleted the noahtallen:patch-1 branch Sep 12, 2019
dd32 pushed a commit to dd32/gutenberg that referenced this pull request Sep 27, 2019
* Add disable support to PostStatus settings panel

While many other panels support the isEditorPanelEnabled option, the
PostStatus panel does not. If the 'post-status' panel is not enabled, the
component now returns null. Previously, it always returned the full
Panel regardless of whether it was enabled.

This is cleaner than returning null, since ifCondition does that for us.

* Use isEditorPanelRemoved instead of isEditorPanelEnabled

We do this since the panel should never be disabled with the enabled
selector, but it can be programmatically removed.

* Update logic for removing post status

* Update e2e test to remove PostStatus panel

* Update e2e test to check if Status Panel exists
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.