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

MetaBoxes: Fix meta boxes toggling #5902

Merged
merged 2 commits into from Apr 5, 2018

Conversation

@youknowriad
Copy link
Contributor

youknowriad commented Mar 30, 2018

Some MetaBoxes depend on the post script which will have the side effect of calling window.postboxes.add_postbox_toggles( postType ) twice. This breaks meta boxes collapsing.

In this PR, I only init the postboxes if it hasn't been done yet.

Testing instructions

  • Install the EditFlow plugin
  • Collapse/Uncollapse the meta boxes
  • It should work as intended.

@youknowriad youknowriad self-assigned this Mar 30, 2018

@youknowriad youknowriad requested a review from WordPress/gutenberg-core Mar 30, 2018

@youknowriad youknowriad added this to the 2.6 milestone Mar 30, 2018

if ( window.postboxes.page !== postType ) {
window.postboxes.add_postbox_toggles( postType );
}
} );

This comment has been minimized.

@danielbachhuber

danielbachhuber Mar 30, 2018

Member

Given how key this is to the core WordPress experience, is it possible to add a test for it?

This comment has been minimized.

@youknowriad

youknowriad Mar 30, 2018

Author Contributor

With a lot of mocks, I can probably add a unit test but I don't think it will be a valuable test because this test would test the implementation and not really the problem.

I think an e2e test is better here but we have an issue :) We didn't do tests that install plugins yet I'm going to try.

This comment has been minimized.

@danielbachhuber

danielbachhuber Mar 30, 2018

Member

We didn't do tests that install plugins yet I'm going to try.

I don't know that I'd install a third-party plugin (because that's likely to change over time). I'd probably create a small meta box PHP file and include it in the tests directory for occasional inclusion.

This comment has been minimized.

@youknowriad

youknowriad Mar 30, 2018

Author Contributor

I'm planning to use EditFlow for this test, Granted that it can change but I'm thinking not that often. If you'd like to help with adding a test plugin (also not certain how to enable this plugin in our current docker setup), I'd really appreciate it.

This comment has been minimized.

@danielbachhuber

danielbachhuber Mar 30, 2018

Member

I've created #5909. My comments shouldn't block merge

@aduth
Copy link
Member

aduth left a comment

Ideally postbox.js should be resilient and avoid double-binding, and/or post.js should do the same as proposed here and not call to add toggle bindings if already configured. That we're forced to use setTimeout is... not ideal.

Further (but unrelated), in what way is this a store side effect? This seems like a UI concern, perhaps something handled by one of the meta box components. And while it should apply globally, we could memoize / once this accordingly if there's a concern of multiple instances of MetaBoxes / MetaBoxArea.

@youknowriad

This comment has been minimized.

Copy link
Contributor Author

youknowriad commented Apr 4, 2018

I think this should be kept as a global side effect because it doesn't receive any "container" DOM node to bind its events. and I agree about postbox.js resilient to double-binding.

I think this PR is one of these areas that would be improved when we merge into Core. I don't want to over optimize it at the moment.

@aduth

This comment has been minimized.

Copy link
Member

aduth commented Apr 4, 2018

I think this PR is one of these areas that would be improved when we merge into Core.

How can we ensure this will happen?

@youknowriad

This comment has been minimized.

Copy link
Contributor Author

youknowriad commented Apr 4, 2018

@aduth I can clarify this in the comment?

@aduth

This comment has been minimized.

Copy link
Member

aduth commented Apr 4, 2018

Seems fine.

@youknowriad youknowriad merged commit 576a757 into master Apr 5, 2018

2 checks passed

codecov/project 44.33% (+0.13%) compared to 3a5dd2d
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@youknowriad youknowriad deleted the fix/metabox-toggle-postboxes branch Apr 5, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment