Add Custom Fields and Advanced Panels to Options modal by noisysocks · Pull Request #10676 · WordPress/gutenberg

noisysocks · 2018-10-17T06:10:56Z

What this does

Closes #10210 and closes #3228.

Firstly, this PR allows the user to enable or disable Advanced Panels (AKA Meta Boxes) via the Screen Options modal that was added in #10215.

Secondly, it adds Custom Fields to Gutenberg in the form of an Advanced Panel that is disabled by default.

How it works

The title and ID of each Meta Box is stored in the core/edit-post Redux store.
The MetaBoxTitles HOC provides an easy way to list all of the registered Meta Boxes within Gutenberg.
The MetaBoxesVisibility and MetaBoxVisibility components respond to the existing isEditorPanelEnabled selector and show/hide Meta Boxes using el.style.display = 'none';
The Custom Fields Meta Box (postcustom) is no longer filtered out by gutenberg_filter_meta_boxes

How to test

Create a new post
Click on the More menu and select Options
Enable Custom Fields—the Custom Fields meta box should appear and be usable
Activate a plugin that adds a Meta Box, e.g. ACF
Create a new post
You should be able to toggle on/off the panels that were added by the plugin

Remaining tasks

Fix issue which causes Custom Fields panel to be enabled after the user upgrades Gutenberg
Prevent empty .edit-post-layout__metaboxes container from appearing when all Advanced Panels are disabled
Tests and documentation

noisysocks · 2018-10-17T06:16:14Z

@youknowriad @aduth @gziolo: This is a first pass at completing #10210. Would greatly appreciate your thoughts on the overall direction¹ before I work on adding tests and documentation.

¹ Spoiler: it's hacky as hell 🙂

Allows the user to enable or disable 'Advanced Panels' (AKA Meta Boxes) via the Options modal. - The title and ID of each Meta Box is stored in the `core/edit-post` Redux store. - The `MetaBoxTitles` HOC provides an easy way to list all of the registered Meta Boxes within Gutenberg. - The `MetaBoxesVisibility` and `MetaBoxVisibility` components respond to the existing `isEditorPanelEnabled` selector and show/hide Meta Boxes using `el.style.display = 'none'`;

Re-enable the Custom Fields Meta Box which is included in Core, but have the panel disabled by default.

noisysocks · 2018-10-18T01:59:58Z

I've added the ability to enable and disable Custom Fields (disabled by default) and given the PR a more detailed description.

Once I have a 👍 on the general direction here I'll start on the remaining tasks which are outlined above.

gziolo · 2018-10-18T07:10:19Z

I think the general direction is very good. It isn't an easy task to get more control over Meta Boxes :) @youknowriad spent weeks polishing the way it works and integrates with Gutenberg. I will leave some comments on the areas where I feel needs to be well documented to make it easier to maintain in the future.

My first impression is that it should be emphasized that MetaBoxesVisibility is a virtual component which doesn't render anything. I had also to spent a few minutes understanding MetaBoxTitles, it's a nice abstraction which calls render callback on each title. Maybe it would help to rename prop to renderItem to better express intent:

<MetaBoxTitles
	renderItem={ ( title, id ) => (
		<EnablePanelOption label={ title } panelName={ `meta-box-${ id }` } />
	) }
/>

We use this pattern in other places like <Dropdown /> where we have renderToggle and renderContent. See https://github.com/WordPress/gutenberg/tree/master/packages/components/src/dropdown.

youknowriad

Left some comments, the general approach is good for me. My comments are mainly refactoring comments that can be done separately if needed.

youknowriad · 2018-10-18T07:15:05Z

lib/meta-box-partial-page.php

 	 */
 	$script = 'window._wpLoadGutenbergEditor.then( function() {
 		wp.data.dispatch( \'core/edit-post\' ).setActiveMetaBoxLocations( ' . wp_json_encode( $active_meta_box_locations ) . ' );
+		wp.data.dispatch( \'core/edit-post\' ).setMetaBoxTitles( ' . wp_json_encode( $meta_box_titles ) . ' );


The name feels a bit weird to me. Is this really setMetaBoxTitles or more something like setAvailableMetaBoxes?
Also, maybe more logical to invert these two dispatches, as we should initialize the available metaboxes first and then tell it which one is active?

We probably could also consolidate into one action since you proposed to have only one reducer.

youknowriad · 2018-10-18T07:17:54Z

packages/edit-post/src/components/layout/index.js

 				<div className="edit-post-layout__metaboxes">
 					<MetaBoxes location="advanced" />
 				</div>
+				<MetaBoxesVisibility />


Is this really useful as its own isolated component? Should this be moved inside the MetaBoxes component instead? Granted, it will force us to filter it per location.

youknowriad · 2018-10-18T07:22:53Z

packages/edit-post/src/store/reducer.js

+	}
+
+	return state;
+}


Can we do a small refactoring to all these meta box states? I think we should just unify under a unique reducer (or combineReducers). Something like:

const metaboxes = { isSaving: true, locations: { side: { isActive: true, metaboxes: [ // this could also be keyed by id { id: 'yoast', label: 'Yoast' } ] } } }

It's not crucial though but I feel we'd gain in clarity by rethinking these states. Thoughts?

youknowriad · 2018-10-18T07:23:10Z

packages/edit-post/src/store/selectors.js

 	return getActiveMetaBoxLocations( state ).includes( location );
 }

+export function getMetaBoxTitles( state ) {


Needs a JSDoc (and doc generation)

youknowriad · 2018-10-18T07:23:27Z

packages/edit-post/src/store/actions.js

 	};
 }

+export function setMetaBoxTitles( titles ) {


Need JSDoc and Doc generation

youknowriad · 2018-10-18T07:30:11Z

packages/edit-post/src/components/meta-box-titles/index.js

+import { Fragment } from '@wordpress/element';
+import { withSelect } from '@wordpress/data';
+
+function MetaBoxTitles( { titles, titleWrapper } ) {


Actually, I'm not certain what value this component brings, as it seems like we already have the selector as an abstraction to get the meta-boxes titles

youknowriad · 2018-10-18T09:13:24Z

I'm going to try to push this to the finish line according to my comments to land in 4.1. I hope you agree with those @noisysocks

youknowriad · 2018-10-18T10:06:47Z

Actually, I think there's a conceptual issue in this PR because we don't update the "active metabox locations" when we show/hide meta boxes. I'm going to try to fix it though.

youknowriad · 2018-10-18T11:05:47Z

I've updated the PR.

We can't add the "custom fields" meta box yet because this would have the side effect of adding a new "save" request for all Gutenberg installs and we want to optimize for metabox-less experience. So we'd likely need to handle it as a special case in this modal (triggering this option or button would reload the page with the meta box enabled)

I'd appreciate some manual testing with ACF for instance.

I still need to add tests and polish deprecations.

gziolo

We should audit all updated selectors and ensure all those which return arrays won't cause performance issues.

gziolo · 2018-10-18T14:00:32Z

packages/edit-post/src/store/selectors.js

+	return get(
+		state.metaBoxes.locations,
+		[ location ],
+		[]


The default value will have a different reference on every call.

We probably should have some a reusable const or helper which always returns the same instance of an empty array or object to avoid re-renders on every call.

gziolo · 2018-10-18T14:01:46Z

packages/edit-post/src/store/selectors.js

+ *
+ * @return {Array} List of meta boxes.
+ */
+export function getAllMetaBoxes( state ) {


Should we use memoization helper here?

Memoization should be avoided if possible to avoid by shaping state such that we don't need to filter / map over it.

gziolo · 2018-10-18T14:02:36Z

packages/edit-post/src/store/selectors.js

 */
 export function getActiveMetaBoxLocations( state ) {
-	return state.activeMetaBoxLocations;
+	return Object.keys( state.metaBoxes.locations )


Should we use memoization here?

aduth · 2018-10-18T14:01:33Z

packages/edit-post/src/components/meta-boxes/index.js

+	return (
+		<Fragment>
+			{ map( metaBoxes, ( { id } ) => (
+				<MetaBoxVisibility id={ id } />


Does this not demand a key?

Edit: Yes, there's a console error.

It does, copy/paste :(

aduth · 2018-10-18T14:15:48Z

packages/edit-post/src/store/selectors.js

+	return get(
+		state.metaBoxes.locations,
+		[ location ],
+		[]


This will return a new array each time (i.e. busting component purity). Previously I've used a top-of-scope constant variable.

aduth · 2018-10-18T14:16:25Z

packages/edit-post/src/store/selectors.js

+			allMetaBoxes = allMetaBoxes.concat( metaBoxes );
+		} );
+
+	return allMetaBoxes;


Another new-array-each-time. Could state shape be optimized for direct return?

No matter the shape of the reducer, it will impact one selector or the other.

aduth · 2018-10-18T14:24:03Z

Seems to be an issue on master as well, but ACF meta values aren't saving and a dirty prompt is shown when trying to reload.

youknowriad · 2018-10-18T14:25:20Z

@aduth Funny because I was testing with an old version of ACF and it was working fine :)

youknowriad · 2018-10-18T14:30:23Z

I'm going to consider it as an ACF bug that's out of scope of the current PR. It was also mentioned here #10660.

aduth · 2018-10-18T14:33:22Z

packages/edit-post/src/store/selectors.js

 */
 export function isMetaBoxLocationActive( state, location ) {
-	return getActiveMetaBoxLocations( state ).includes( location );
+	return getMetaBoxesPerLocation( state, location ).length !== 0;


getMetaBoxesPerLocation is not guaranteed to return an array, at which point this throws an error.

Should be fine

aduth · 2018-10-18T14:57:08Z

packages/edit-post/src/components/meta-boxes/index.js

Is it necessary to render this component at all if ! isVisible? i.e. an ifCondition.

It's necessary to handle the visibility.

aduth · 2018-10-18T14:58:22Z

packages/edit-post/src/store/selectors.js

Possible simplification?

return flatten( values( state.metaBoxes.locations ) );

youknowriad · 2018-10-18T15:46:47Z

Thanks for your help on this.

@noisysocks Let's try to tackle #3228 as a follow-up using this approach or similar #10676 (comment)

noisysocks · 2018-10-18T22:58:13Z

Thanks for taking over @youknowriad! I love the approach of handling everything in MetaBoxes.

@noisysocks Let's try to tackle #3228 as a follow-up using this approach or similar #10676 (comment)

👍 Will look into this.

* First pass at adding 'Advanced Panels' to Options modal Allows the user to enable or disable 'Advanced Panels' (AKA Meta Boxes) via the Options modal.

noisysocks added [Status] In Progress Tracking issues with work in progress Backwards Compatibility Issues or PRs that impact backwards compatability labels Oct 17, 2018

mtias added the [Feature] Meta Boxes A draggable box shown on the post editing screen label Oct 17, 2018

noisysocks force-pushed the try/advanced-panel-screen-options branch from f864dc0 to 1779ddc Compare October 17, 2018 23:04

noisysocks added this to the 4.1 - UI freeze milestone Oct 17, 2018

Allow Custom Fields to be enabled via Screen Options

8bb0f1f

Re-enable the Custom Fields Meta Box which is included in Core, but have the panel disabled by default.

noisysocks changed the title ~~First pass at adding 'Advanced Panels' to Options modal~~ Add Custom Fields and Advanced Panels to Options modal Oct 18, 2018

noisysocks requested review from aduth, gziolo and youknowriad and removed request for gziolo and youknowriad October 18, 2018 01:58

youknowriad reviewed Oct 18, 2018

View reviewed changes

Refactor metaboxes visibility

e24a833

youknowriad added 3 commits October 18, 2018 12:35

Document selectors and actions

f7d3687

Unit tests

e0f34b0

Return valid actions from action creators

59ca37a

youknowriad self-assigned this Oct 18, 2018

youknowriad removed the [Status] In Progress Tracking issues with work in progress label Oct 18, 2018

Fix more tests

2cab52a

gziolo reviewed Oct 18, 2018

View reviewed changes

aduth reviewed Oct 18, 2018

View reviewed changes

youknowriad added 2 commits October 18, 2018 15:20

Memoize selectors to avoid new instances on each call

974ef8d

missing key

57ef63c

aduth previously requested changes Oct 18, 2018

View reviewed changes

aduth reviewed Oct 18, 2018

View reviewed changes

aduth approved these changes Oct 18, 2018

View reviewed changes

Fix small bug in isMetaBoxLocationActive

673750e

youknowriad force-pushed the try/advanced-panel-screen-options branch from 7aba1e3 to 673750e Compare October 18, 2018 15:00

Tweak getAllMetaBoxes selectors

113dddf

youknowriad merged commit cbf425b into master Oct 18, 2018

youknowriad deleted the try/advanced-panel-screen-options branch October 18, 2018 15:45

ocean90 mentioned this pull request Oct 21, 2018

Avoid PHP notices due to non-available meta boxes #10850

Merged

noisysocks mentioned this pull request Oct 25, 2018

Should WP Core "Custom Fields" UI be present by default? #3228

Closed

2 tasks

mcsf mentioned this pull request Oct 25, 2018

Version 4.1 Shows All Post Meta Field Groups (using ACF) #11071

Closed

noisysocks mentioned this pull request Oct 26, 2018

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

Merged

noisysocks mentioned this pull request Nov 5, 2018

Custom Fields option displays when ACF is enabled #11386

Closed

noisysocks mentioned this pull request Dec 5, 2018

ACF Metaboxes are always visible #12571

Closed

aduth mentioned this pull request Dec 6, 2018

Meta Boxes: Don't hide disabled meta boxes by modifying DOM #12628

Merged

noisysocks mentioned this pull request May 8, 2019

Custom field check box automatically changes the context on user focus #15320

Closed

Conversation

noisysocks commented Oct 17, 2018 • edited by youknowriad Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What this does

How it works

How to test

Remaining tasks

Uh oh!

noisysocks commented Oct 17, 2018

Uh oh!

noisysocks commented Oct 18, 2018

Uh oh!

gziolo commented Oct 18, 2018

Uh oh!

youknowriad left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

youknowriad commented Oct 18, 2018

Uh oh!

youknowriad commented Oct 18, 2018

Uh oh!

youknowriad commented Oct 18, 2018

Uh oh!

gziolo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aduth commented Oct 18, 2018

Uh oh!

youknowriad commented Oct 18, 2018

Uh oh!

youknowriad commented Oct 18, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

youknowriad commented Oct 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

noisysocks commented Oct 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

noisysocks commented Oct 17, 2018 •

edited by youknowriad

Loading

youknowriad commented Oct 18, 2018 •

edited

Loading

noisysocks commented Oct 18, 2018 •

edited

Loading