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

Advanced Panels: Add support for the 'Custom Fields' meta box #11084

Merged
merged 9 commits into from Nov 2, 2018

Conversation

noisysocks
Copy link
Member

@noisysocks noisysocks commented Oct 26, 2018

Closes #3228.

custom-fields

Adds an option which allows the user to enable the existing 'postcustom' meta box included in WordPress.

I've implemented this using the super hacky approach described in #10676 (comment) 😎

The benefits of this approach are that:

  • We can reuse the existing 'postcustom' meta box.
  • The 'postcustom' assets aren't loaded unless the option is enabled.

The downsides are that:

  • We have to store the option on the server (e.g. using a site option).
  • The editor has to be reloaded when the option changes, which is pretty jarring.

@noisysocks noisysocks added [Feature] Meta Boxes A draggable box shown on the post editing screen Backwards Compatibility Issues or PRs that impact backwards compatability labels Oct 26, 2018
Copy link
Contributor

@youknowriad youknowriad left a comment

Choose a reason for hiding this comment

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

This makes sense unless we have REST API support for custom fields which I don't really expect it to come in 5.0 (or later?)

I'm adding some reviewers more familiar with the PHP side of things.

Copy link
Member

@danielbachhuber danielbachhuber left a comment

Choose a reason for hiding this comment

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

Looking good! Few things to address.

lib/meta-box-partial-page.php Outdated Show resolved Hide resolved
lib/meta-box-partial-page.php Outdated Show resolved Hide resolved
@@ -1573,14 +1573,27 @@ function gutenberg_editor_scripts_and_styles( $hook ) {
'maxUploadFileSize' => $max_upload_size,
Copy link
Member

Choose a reason for hiding this comment

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

It works!

image

Should I be seeing that error message though?

Copy link
Contributor

Choose a reason for hiding this comment

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

I saw the error mentioned for other meta boxes elsewhere. I wonder if it's a more generic bug in 5.0?

Copy link
Member Author

Choose a reason for hiding this comment

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

How did you get this error?

Copy link
Member

Choose a reason for hiding this comment

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

It's a bug in 5.0, fixed now.

lib/meta-box-partial-page.php Show resolved Hide resolved
@noisysocks
Copy link
Member Author

Noting to myself that since this touches the PHP we'll need to bring over these changes to the 5.0 branch.

@noisysocks noisysocks dismissed danielbachhuber’s stale review October 29, 2018 23:10

Feedback has been addressed 🙂

@noisysocks
Copy link
Member Author

How do we feel about this? Would like to get it into the 5.0 RC.

Copy link
Contributor

@talldan talldan left a comment

Choose a reason for hiding this comment

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

I had a look at the front-end side of things. The changes seem to be pretty minor there. Looks good to me - seems mergeable if the backend changes also look acceptable.

@websupporter
Copy link

How does this approach take into account a custom post type where custom-fields are not supported, like for example

add_action('init', function() {
        remove_post_type_support( 'post', 'custom-fields' );
});

I would expect the option to not show up in the first place, but from a quick view into the code, I can't see a toggle for that.

In the classic editor, when I go to the screen options, the "Custom Fields"-option disappears when the support has been removed.

- Adds an option which allows the user to enable the existing
  'postcustom' meta box included in WordPress.
- Don't load the 'postcustom' assets when the option is disabled.
- Stores the option as a site option.
- Reload the editor when the option is changed.
The 'Enable Tips' checkbox is obscured by the first Tip now that the
Options modal is taller. Work around this by only testing that tips can
be re-enabled.
We can simply interpolate $post->ID when initializing Custom Fields,
rather than selecting from the #post_ID hidden field.
By using `get_user_meta()` instead of `get_option()`, the Custom Fields
setting won't affect other users.
Trigger the toggle_custom_fields action using a hidden <form> so that
the action is triggered by a POST request rather than a GET request.
@noisysocks noisysocks force-pushed the try/custom-fields-meta-box branch 2 times, most recently from 292485a to c01629c Compare November 2, 2018 01:28
@noisysocks
Copy link
Member Author

Thanks all. I've responded to feedback and fixed the problem where Custom Fields would display in post types that don't support postcustom.

One issue I've noticed: when the Advanced Custom Fields plugin is enabled, the Custom Fields toggle is displayed even though it doesn't do anything. This is because the ACF plugin removes postcustom from the list of registered meta boxes. I don't think it's a blocker for this PR. Likely the fix is that ACF, when enabled, will have to mark the post type as not supporting custom-fields.

@noisysocks noisysocks added this to the 4.3 milestone Nov 2, 2018
Copy link
Contributor

@talldan talldan left a comment

Choose a reason for hiding this comment

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

Yep, it's undergone a lot of reviews and feedback looks all addressed 👍

@noisysocks
Copy link
Member Author

🙈

@noisysocks noisysocks merged commit e30f36f into master Nov 2, 2018
@noisysocks noisysocks deleted the try/custom-fields-meta-box branch November 2, 2018 03:12
@noisysocks
Copy link
Member Author

One issue I've noticed: when the Advanced Custom Fields plugin is enabled, the Custom Fields toggle is displayed even though it doesn't do anything. This is because the ACF plugin removes postcustom from the list of registered meta boxes. I don't think it's a blocker for this PR. Likely the fix is that ACF, when enabled, will have to mark the post type as not supporting custom-fields.

Created a follow-up issue for this here: #11386

@noisysocks
Copy link
Member Author

Lastly, created a ticket to remind ourselves to copy over the necessary PHP changes to Core: https://core.trac.wordpress.org/ticket/45257

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Backwards Compatibility Issues or PRs that impact backwards compatability [Feature] Meta Boxes A draggable box shown on the post editing screen
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants