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 Custom Fields and Advanced Panels to Options modal #10676

Merged
merged 11 commits into from Oct 18, 2018

Conversation

@noisysocks
Member

noisysocks commented Oct 17, 2018

What this does

advanced-panels

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

  1. Create a new post
  2. Click on the More menu and select Options
  3. Enable Custom Fields—the Custom Fields meta box should appear and be usable
  4. Activate a plugin that adds a Meta Box, e.g. ACF
  5. Create a new post
  6. 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

This comment has been minimized.

Member

noisysocks commented Oct 17, 2018

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

1 Spoiler: it's hacky as hell 🙂

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.

- 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'`;

@noisysocks noisysocks force-pushed the try/advanced-panel-screen-options branch from f864dc0 to 1779ddc Oct 17, 2018

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

Allow Custom Fields to be enabled via Screen Options
Re-enable the Custom Fields Meta Box which is included in Core, but
have the panel disabled by default.

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

@noisysocks noisysocks requested review from gziolo, youknowriad and aduth and removed request for gziolo and youknowriad Oct 18, 2018

@noisysocks

This comment has been minimized.

Member

noisysocks commented Oct 18, 2018

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

This comment has been minimized.

Member

gziolo commented Oct 18, 2018

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.

@@ -328,6 +340,7 @@ function the_gutenberg_metaboxes() {
*/
$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 ) . ' );

This comment has been minimized.

@youknowriad

youknowriad Oct 18, 2018

Contributor

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?

This comment has been minimized.

@gziolo

gziolo Oct 18, 2018

Member

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

@@ -91,6 +92,7 @@ function Layout( {
<div className="edit-post-layout__metaboxes">
<MetaBoxes location="advanced" />
</div>
<MetaBoxesVisibility />

This comment has been minimized.

@youknowriad

youknowriad Oct 18, 2018

Contributor

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.

}
return state;
}

This comment has been minimized.

@youknowriad

youknowriad Oct 18, 2018

Contributor

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?

@@ -239,6 +239,10 @@ export function isMetaBoxLocationActive( state, location ) {
return getActiveMetaBoxLocations( state ).includes( location );
}
export function getMetaBoxTitles( state ) {

This comment has been minimized.

@youknowriad

youknowriad Oct 18, 2018

Contributor

Needs a JSDoc (and doc generation)

@@ -220,6 +220,13 @@ export function setActiveMetaBoxLocations( locations ) {
};
}
export function setMetaBoxTitles( titles ) {

This comment has been minimized.

@youknowriad

youknowriad Oct 18, 2018

Contributor

Need JSDoc and Doc generation

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

This comment has been minimized.

@youknowriad

youknowriad Oct 18, 2018

Contributor

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

This comment has been minimized.

Contributor

youknowriad commented Oct 18, 2018

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

This comment has been minimized.

Contributor

youknowriad commented Oct 18, 2018

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

This comment has been minimized.

Contributor

youknowriad commented Oct 18, 2018

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.

youknowriad added some commits Oct 18, 2018

@gziolo

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

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

This comment has been minimized.

@gziolo

gziolo Oct 18, 2018

Member

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.

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

This comment has been minimized.

@gziolo

gziolo Oct 18, 2018

Member

Should we use memoization helper here?

This comment has been minimized.

@aduth

aduth Oct 18, 2018

Member

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

@@ -223,7 +223,25 @@ export function getMetaBoxes( state ) {
* @return {string[]} Active meta box locations.
*/
export function getActiveMetaBoxLocations( state ) {
return state.activeMetaBoxLocations;
return Object.keys( state.metaBoxes.locations )

This comment has been minimized.

@gziolo

gziolo Oct 18, 2018

Member

Should we use memoization here?

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

This comment has been minimized.

@aduth

aduth Oct 18, 2018

Member

Does this not demand a key?

Edit: Yes, there's a console error.

This comment has been minimized.

@youknowriad

youknowriad Oct 18, 2018

Contributor

It does, copy/paste :(

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

This comment has been minimized.

@aduth

aduth Oct 18, 2018

Member

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

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

This comment has been minimized.

@aduth

aduth Oct 18, 2018

Member

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

This comment has been minimized.

@youknowriad

youknowriad Oct 18, 2018

Contributor

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

youknowriad added some commits Oct 18, 2018

@aduth

This comment has been minimized.

Member

aduth commented Oct 18, 2018

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

This comment has been minimized.

Contributor

youknowriad commented Oct 18, 2018

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

@youknowriad

This comment has been minimized.

Contributor

youknowriad commented Oct 18, 2018

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.

@@ -236,9 +260,43 @@ export function getActiveMetaBoxLocations( state ) {
* @return {boolean} Whether the meta box location is active.
*/
export function isMetaBoxLocationActive( state, location ) {
return getActiveMetaBoxLocations( state ).includes( location );
return getMetaBoxesPerLocation( state, location ).length !== 0;

This comment has been minimized.

@aduth

aduth Oct 18, 2018

Member

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

Should be fine

{ map( metaBoxes, ( { id } ) => (
<MetaBoxVisibility key={ id } id={ id } />
) ) }
{ isVisible && <MetaBoxesArea location={ location } /> }

This comment has been minimized.

@aduth

aduth Oct 18, 2018

Member

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

This comment has been minimized.

@youknowriad

youknowriad Oct 18, 2018

Contributor

It's necessary to handle the visibility.

*/
export const getAllMetaBoxes = createSelector(
( state ) => {
let allMetaBoxes = [];

This comment has been minimized.

@aduth

aduth Oct 18, 2018

Member

Possible simplification?

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

This comment has been minimized.

@youknowriad

youknowriad Oct 18, 2018

Contributor

Definitely

@aduth

aduth approved these changes Oct 18, 2018

@youknowriad youknowriad force-pushed the try/advanced-panel-screen-options branch from 7aba1e3 to 673750e Oct 18, 2018

@youknowriad youknowriad merged commit cbf425b into master Oct 18, 2018

2 checks passed

codecov/project 49.44% (-0.03%) compared to c3501c7
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@youknowriad youknowriad deleted the try/advanced-panel-screen-options branch Oct 18, 2018

@youknowriad

This comment has been minimized.

Contributor

youknowriad commented Oct 18, 2018

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

This comment has been minimized.

Member

noisysocks commented Oct 18, 2018

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.

antpb added a commit to antpb/gutenberg that referenced this pull request Oct 26, 2018

Add Custom Fields and Advanced Panels to Options modal (WordPress#10676)
* 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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment