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

Adds Metabox Support. #2804

Merged
merged 5 commits into from Oct 19, 2017
Merged

Adds Metabox Support. #2804

merged 5 commits into from Oct 19, 2017

Conversation

@BE-Webdesign
Copy link
Contributor

@BE-Webdesign BE-Webdesign commented Sep 27, 2017

Description

This adds metabox support with the UI @aduth came up with. See the accompanying markdown file in the docs.

How Has This Been Tested?

Tested pretty extensively. Lots more polish work to do, but I believe this is merge-able-ish.

Screenshots (jpeg or gifs if applicable):

Types of changes

Adds PHP metabox support, through incredible hacks.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows has proper inline documentation.

Testing Instructions:

  • Verify that by default, on an install with no custom metaboxes, that no changes occur to the current Gutenberg experience.
  • Verify that all core metaboxes do not appear, and no Extended Settings areas appear.
  • Install ACF, Metabox.io, CMB2, or some other metabox creation framework, add their metaboxes to your theme, or generate some metaboxes of your own.
  • Verify that metaboxes render in the Extended Settings area of your choice. There are normal, advanced, and side areas. advanced is currently merged into normal.
  • Verify that normal metaboxes appear at the bottom of the editor area in an Editor Settings toggle panel.
  • If you register sidebar metaboxes, verify that an 'Extended Settings' area appears in the sidebar.
  • Verify that changing the form values in the metaboxes will allow you to update the post.
  • Verify that returning the state of the form values to their original value will mark the metabox state as not dirty.
  • When saving a post without changing metabox values, verify that they do not update.
  • When saving a post with metabox changes, verify that the metaboxes indeed save the settings.
  • If the sidebar only has changes and the normal area does not, verify that only the side bar saves, and vice versa.
  • After successfully updating and verify the values saved correctly, try continual updates. The form data to dirty check against should now be the updated data.
@BE-Webdesign
Copy link
Contributor Author

@BE-Webdesign BE-Webdesign commented Sep 27, 2017

Probably wouldn't merge this as it technically works, but there are still some rough edges. No sense pushing a commit on a touchy subject that is not fully 100% there, but I leave that decision up to y'all.

@codecov
Copy link

@codecov codecov bot commented Sep 27, 2017

Codecov Report

Merging #2804 into master will decrease coverage by 0.18%.
The diff coverage is 17.68%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #2804      +/-   ##
=========================================
- Coverage   32.59%   32.4%   -0.19%     
=========================================
  Files         209     211       +2     
  Lines        5965    6114     +149     
  Branches     1055    1078      +23     
=========================================
+ Hits         1944    1981      +37     
- Misses       3391    3485      +94     
- Partials      630     648      +18
Impacted Files Coverage Δ
editor/sidebar/index.js 0% <ø> (ø) ⬆️
editor/layout/index.js 0% <0%> (ø) ⬆️
editor/meta-boxes/panel.js 0% <0%> (ø)
editor/index.js 0% <0%> (ø) ⬆️
editor/sidebar/post-settings/index.js 0% <0%> (ø) ⬆️
editor/meta-boxes/iframe.js 0% <0%> (ø)
editor/meta-boxes/index.js 0% <0%> (ø) ⬆️
editor/reducer.js 89.94% <100%> (+0.9%) ⬆️
editor/selectors.js 95.65% <100%> (+0.22%) ⬆️
editor/effects.js 51.31% <100%> (+11.31%) ⬆️
... and 6 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 15c515b...bab6410. Read the comment docs.

@@ -17,11 +17,12 @@ import Header from '../header';
import Sidebar from '../sidebar';
import TextEditor from '../modes/text-editor';
import VisualEditor from '../modes/visual-editor';
import MetaBoxes from '../meta-boxes';
// import MetaBoxes from '../meta-boxes';

This comment has been minimized.

@youknowriad

youknowriad Sep 27, 2017
Contributor

Forgot a comment?

@jasmussen
Copy link
Contributor

@jasmussen jasmussen commented Sep 27, 2017

Thank you for working on this 🌟🌟🌟🌟🌟

What's the best way to test this PR? Is there a particular metabox plugin that would be good to install to test?

@youknowriad
Copy link
Contributor

@youknowriad youknowriad commented Sep 27, 2017

@jasmussen I tested using ACF which seems to work properly

@youknowriad
Copy link
Contributor

@youknowriad youknowriad commented Sep 27, 2017

but there are still some rough edges

Could you elaborate on this, are you talking about the sync (Yoast)?

@BE-Webdesign
Copy link
Contributor Author

@BE-Webdesign BE-Webdesign commented Sep 27, 2017

but there are still some rough edges
Could you elaborate on this, are you talking about the sync (Yoast)?

Nope, has to do with timing of saving the metabox form. If you are really quick, you can hit update, change a field value before the post saves, and then that change will be saved "in-flight" I was thinking about putting a "updating" visual state, both when the iframe initially loads, and when it is saving. It would be some sort of overlay with a spinner.

@BE-Webdesign
Copy link
Contributor Author

@BE-Webdesign BE-Webdesign commented Sep 27, 2017

What's the best way to test this PR? Is there a particular metabox plugin that would be good to install to test?

Anything except Yoast.

Copy link
Contributor

@mcsf mcsf left a comment

Thanks for working on this! I'll need to come back to this for a better review, but here's some preliminary feedback.

One thing I'm struggling with is the lifecycle in and around MetaboxIframe. For one, subscribing to events — some common patterns are:

  • My component is always listening to x, so component(Will/Did)Mount will feature a addEventListener for my bound handler and componentWillUnmount will feature a removeEventListener.

  • My component supports swapping in such a way that somewhere some atom will look like removeEventListener( ...oldEntities ); swapEntities(); addEventListener( ...newEntities );.

These, IMO, are not very apparent, and it's hard to statically look at MetaboxIframe and be sure that nothing is leaking, that the lifecycle is sound, etc. As a reviewer, this means I'm much less confident that I fully understand the solution.

@@ -113,6 +115,10 @@ export default {
) );
}

// Update dirty metaboxes.
const metaboxes = getDirtyMetaboxes( getState() );
metaboxes.map( metabox => dispatch( requestMetaboxUpdate( metabox ) ) );

This comment has been minimized.

@mcsf

mcsf Sep 27, 2017
Contributor

A minor point, but .forEach carries better meaning than .map, as the intent is to perform side effects and we are not keeping the mapped collection.

This comment has been minimized.

@BE-Webdesign

BE-Webdesign Sep 27, 2017
Author Contributor

Yup, definitely.

);
};

const Metabox = ( props ) => renderMetabox( props );

This comment has been minimized.

@mcsf

mcsf Sep 27, 2017
Contributor

const f = ( x ) => g( x ); is equivalent to f = g. :) I suggest removing this assignment and then renaming renderMetabox to Metabox altogether.

This comment has been minimized.

@BE-Webdesign

BE-Webdesign Sep 27, 2017
Author Contributor

Yeah, I think this is just a left over from it being something different at one point.

if ( ( this.props.isUpdating === false && nextProps.isUpdating === true )
|| ( this.props.isUpdating === true && nextProps.isUpdating === true ) ) {
return false;
}

This comment has been minimized.

@mcsf

mcsf Sep 27, 2017
Contributor

Here we have the expression ( a && b ) || ( ! a && b ), which can be reduced to b. Hence shouldComponentUpdate could be rewritten as

return nextProps.isUpdating === false;

was the repetition intentional, to convey a certain meaning?

This comment has been minimized.

@BE-Webdesign

BE-Webdesign Sep 27, 2017
Author Contributor

was the repetition intentional, to convey a certain meaning?

Nope, just iterative code slapped ontop of what was there before, didn't take anytime to consolidate/look at things.

} catch ( e ) {
return false;
}
}

This comment has been minimized.

@mcsf

mcsf Sep 27, 2017
Contributor

Would something like has( this, 'node.contentDocument.body' ) (or !! get( this, 'node.contentDocument.body' )) be more fitting than the more muscular try/catch construct?

This comment has been minimized.

@BE-Webdesign

BE-Webdesign Sep 27, 2017
Author Contributor

Sounds good. We are using the try/catch somewhere else in this. I think in embeds, was trying to keep consistent.

<iframe
ref={ ( node ) => {
this.node = node;
} }

This comment has been minimized.

@mcsf

mcsf Sep 27, 2017
Contributor

AFAIK it's recommended to make this an instance method, then use ref={ this.bindNode }.

This comment has been minimized.

@BE-Webdesign

BE-Webdesign Sep 27, 2017
Author Contributor

Yeah, saves an extra function creation.

This comment has been minimized.

@BE-Webdesign

BE-Webdesign Sep 29, 2017
Author Contributor

Did not know this would call twice, could be the source of some of the problems I have encountered.

}

observeChanges() {
const node = findDOMNode( this.node );

This comment has been minimized.

@mcsf

mcsf Sep 27, 2017
Contributor

Unless I missed something here, isn't this.node already a DOM node? From what I can see, it is only assigned via React's ref. Compare this line with componentDidMount, where this.node is directly manipulated as a DOM element.

This question also applies to some other uses of findDOMNode( this.node ).

This comment has been minimized.

@BE-Webdesign

BE-Webdesign Sep 27, 2017
Author Contributor

This question also applies to some other uses of findDOMNode( this.node ).

For whatever reason this would not work without findDOMNode, potentially when React is bypassed the ref goes stale or something. Will definitely work on cleaning this up.

This comment has been minimized.

@mcsf

mcsf Sep 27, 2017
Contributor

In case this helps:

React will call the ref callback with the DOM element when the component mounts, and call it with null when it unmounts.

and another caveat:

If the ref callback is defined as an inline function, it will get called twice during updates, first with null and then again with the DOM element. This is because a new instance of the function is created with each render, so React needs to clear the old ref and set up the new one. You can avoid this by defining the ref callback as a bound method on the class, but note that it shouldn't matter in most cases.

if ( this.props.isDirty === true ) {
this.props.changedMetaboxState( this.props.location, false );
}
}

This comment has been minimized.

@mcsf

mcsf Sep 27, 2017
Contributor

    if ( a ) { if ( ! b ) { effect( ! b ); } }
    else if ( b ) { effect( ! b ); }
=
    if ( a && ! b ) { effect( ! b ); }
    else if ( b ) { effect( ! b ); }
=
    if ( ! b && a ) { effect( ! b ); }
    else if ( b ) { effect( ! b ); }
=
    if ( b || a ) { effect( ! b ); }
=
    if ( this.props.isDirty || ! isEqual( this.originalFormData, this.getFormData( this.node ) ) ) {
        this.props.changedMetaboxState( this.props.location, ! this.props.isDirty );
    }

😄

This comment has been minimized.

@BE-Webdesign

BE-Webdesign Sep 27, 2017
Author Contributor

Looks good.

This comment has been minimized.

@BE-Webdesign

BE-Webdesign Sep 29, 2017
Author Contributor

After trying this out, it did not work 100%, We have an ! XOR going on so this becomes:

if ( this.props.isDirty === isEqual( this.originalFormData, this.getFormData( this.node ) ) ) {
        this.props.changedMetaboxState( this.props.location, ! this.props.isDirty );
}
'gutenberg-metabox-iframe': true,
[ `${ location }` ]: true,
'sidebar-open': isSidebarOpened,
} );

This comment has been minimized.

@mcsf

mcsf Sep 27, 2017
Contributor

Handy tip: classnames supports classnames( 'gutenberg-metabox-iframe, location, { 'sidebar-open': isSidebarOpened } ).

This comment has been minimized.

@BE-Webdesign

BE-Webdesign Sep 27, 2017
Author Contributor

I knew there had to be a better way, dunno why I couldn't remember to not include everything in the {}.

@BE-Webdesign
Copy link
Contributor Author

@BE-Webdesign BE-Webdesign commented Sep 27, 2017

@mcsf Thank you for the review, I will address some of your points later today.

@BE-Webdesign
Copy link
Contributor Author

@BE-Webdesign BE-Webdesign commented Sep 27, 2017

These, IMO, are not very apparent, and it's hard to statically look at MetaboxIframe and be sure that nothing is leaking, that the lifecycle is sound, etc. As a reviewer, this means I'm much less confident that I fully understand the solution.

Agreed, this will be cleaned up, and I hope to have React always be handling the rendering of the component rather than this weird intermittent hack solution I have running right now.

@BE-Webdesign
Copy link
Contributor Author

@BE-Webdesign BE-Webdesign commented Sep 30, 2017

Note: Add action to easily add styles. Alternative, better solution, provide documentation explaining how to do this with current hooks.

@jasmussen
Copy link
Contributor

@jasmussen jasmussen commented Oct 2, 2017

Again, really nice work here.

I'm not seeing the Extended Settings box at all on the demo content. Not sure if that's expected or not.

Here's a screenshot with Post Meta Inspector and Advanced Custom Fields installed:

screen shot 2017-10-02 at 10 10 36

How much access do we have to the CSS styles of each of these? If we can better provide boundaries around some of these (Post Meta Inspector looks/works pretty well as is), it would be nice.

@BE-Webdesign
Copy link
Contributor Author

@BE-Webdesign BE-Webdesign commented Oct 2, 2017

How much access do we have to the CSS styles of each of these? If we can better provide boundaries around some of these (Post Meta Inspector looks/works pretty well as is), it would be nice.

We have 100% access, we can use editor/metaboxes/metabox-iframe.scss to change any styles that should render inside the iframe. These could also be overridden by plugin and theme authors. Using admin_enqueue_scripts hook with some special conditionals.

I'm not seeing the Extended Settings box at all on the demo content. Not sure if that's expected or not.

For now that is expected, there are special conditionals running for the metaboxes that only are applying to the gutenberg page not the gutenberg-demo page.

Copy link
Member

@karmatosed karmatosed left a comment

I have tested with a range of fields using ACF as an example. Whilst some don't look amazing, I think it's right to add in and that's something plugins will have to iterate on. The big design issues I wanted fixed are, so approving :)

@@ -33,8 +33,8 @@ function Layout( { mode, isSidebarOpened, notices, ...props } ) {
'is-sidebar-opened': isSidebarOpened,
} );

return (
<div className={ className }>
return [

This comment has been minimized.

@mtias

mtias Oct 2, 2017
Contributor

Why is this switched to an array?

This comment has been minimized.

@BE-Webdesign

BE-Webdesign Oct 2, 2017
Author Contributor

I must have missed that while rebasing. The Metabox used to be added after the editor area in an array, now it is inside. Good catch.

@@ -17,11 +17,11 @@ import Header from '../header';
import Sidebar from '../sidebar';
import TextEditor from '../modes/text-editor';
import VisualEditor from '../modes/visual-editor';
import MetaBoxes from '../meta-boxes';

This comment has been minimized.

@mtias

mtias Oct 2, 2017
Contributor

We should keep this writing MetaBox and meta-box :)

This comment has been minimized.

@BE-Webdesign

BE-Webdesign Oct 2, 2017
Author Contributor

Okay, makes sense. I think MetaBoxes + meta-boxes would make the most sense, because the area is a collection of many individual metaboxes?

}
}

.iframe--updating {

This comment has been minimized.

@mtias

mtias Oct 2, 2017
Contributor

We prefer is-modifier classes like is-updating.

This comment has been minimized.

@BE-Webdesign

BE-Webdesign Oct 2, 2017
Author Contributor

Okay, no problem, will change..

This comment has been minimized.

@BE-Webdesign

BE-Webdesign Oct 2, 2017
Author Contributor

I need to probably review the CSS guidelines, and retouch a couple of things within the iframe as well.

Copy link
Member

@aduth aduth left a comment

Well... this pull request was intense. For parts of it, notably the PHP files, I really didn't put as much detail into the review as I would have liked, as I'm not sure what of it was sourced from elsewhere (if any) or the general details around why certain things are included to trick plugins into registering meta boxes.

It's been noted already, but the Yoast plugin really does not work very well with these changes. Something about how it loads immediately flags the post as dirty, and it can become stuck in an infinite loop of saving (with some seconds delay between by autosave). From what I could tell in my brief debugging, there were a handful of meta keys changing after a delayed load. Do you have any thoughts for how we might handle these cases?

var previousWidth, previousHeight;

function sendResize() {
var form = document.getElementById( 'post' );

This comment has been minimized.

@aduth

aduth Oct 2, 2017
Member

Would we expect this reference to change at all over time? Seems we could avoid quite a bit of unnecessary DOM querying by creating this reference in the parent scope. This would also allow us to reuse the reference between here and in observer.observe further down.

This comment has been minimized.

@BE-Webdesign

BE-Webdesign Oct 2, 2017
Author Contributor

Sounds good.

# Metaboxes

This is a brief document detailing how metabox support works in Gutenberg. With
the superior developer and user experience of blocks however, especially once,

This comment has been minimized.

@aduth

aduth Oct 2, 2017
Member

Minor note for readability: Suggest dropping the comma after "once,"


Each metabox area is rendered by a React component containing an iframe.
Each iframe will render a partial page containing only metaboxes for that area.
Metabox data is collected and used for conditional rendering. The metaboxe areas

This comment has been minimized.

@aduth

aduth Oct 2, 2017
Member

Typo: "metaboxe"

original global state is reset upon collection of metabox data.

gutenberg_set_post_state() is hooked in early on admin_head to fake the post
state. This is necessary for ACF to work, no other metabox frameworks seem to

This comment has been minimized.

@aduth

aduth Oct 2, 2017
Member

Wondering if we should include this framework-specific note in here, or if we can generalize this to the specific problem for which we've created the workaround. Something like "in cases where plugins access $post->post_type early in page lifecycle..."


Hooked in later on admin_head is gutenberg_collect_metabox_data(), this will
run through the functions and hooks that post.php runs to register metaboxes;
namely `add_meta_boxes, add_meta_boxes_{$post->post_type}`, and `do_meta_boxes`.

This comment has been minimized.

@aduth

aduth Oct 2, 2017
Member

Minor: Should split the inline code formatting around the comma:

add_meta_boxes, add_meta_boxes_{$post->post_type}, and do_meta_boxes.


add_filter( 'filter_gutenberg_metaboxes', 'gutenberg_filter_metaboxes', 10, 1 );

?>

This comment has been minimized.

@aduth

aduth Oct 2, 2017
Member

Minor: Ending ?> can be omitted

This comment has been minimized.

@BE-Webdesign

BE-Webdesign Oct 2, 2017
Author Contributor

Will get rid of it.


const classes = classnames(
'gutenberg-metabox-iframe',
location,

This comment has been minimized.

@aduth

aduth Oct 2, 2017
Member

Is this class used? Can we treat it as a proper modifier class, is-${ location }?

This comment has been minimized.

@BE-Webdesign

BE-Webdesign Oct 2, 2017
Author Contributor

I don't believe this is used anymore. So I would be in favor of just getting rid of it. Additionally, we should rename the iframe class I imagine as well?

const classes = classnames(
'gutenberg-metabox-iframe',
location,
{ 'sidebar-open': isSidebarOpened }

This comment has been minimized.

@aduth

aduth Oct 2, 2017
Member

Is this class used?

This comment has been minimized.

@BE-Webdesign

BE-Webdesign Oct 2, 2017
Author Contributor

It used to be used, but is probably not relevant anymore.

@@ -113,6 +115,10 @@ export default {
) );
}

// Update dirty metaboxes.
const metaboxes = getDirtyMetaboxes( getState() );
metaboxes.forEach( metabox => dispatch( requestMetaboxUpdate( metabox ) ) );

This comment has been minimized.

@aduth

aduth Oct 2, 2017
Member

Do we have to dispatch these individually, or can we just create a single action which will mark all as needing update?

This comment has been minimized.

@BE-Webdesign

BE-Webdesign Oct 2, 2017
Author Contributor

Not sure what you mean exactly.

This comment has been minimized.

@mcsf

mcsf Oct 4, 2017
Contributor

Something like:

		const metaboxes = getDirtyMetaboxes( getState() );
-		metaboxes.forEach( metabox => dispatch( requestMetaboxUpdate( metabox ) ) );
+		dispatch( requestMetaboxUpdates( metaboxes );
 		case 'REQUEST_META_BOX_UPDATE':
 			return { ...state, [ action.location ]: { ...state[ action.location ], isUpdating: true, isDirty: false } };
+
+		case 'REQUEST_META_BOX_UPDATES':
+			return action.locations.reduce( ( newState, location ) => {
+				newState[ location ] = {
+					...state[ location ],
+					...isUpdating: true,
+					...isDirty: false,
+				};
+				return newState;
+			}, { ...state } );

This comment has been minimized.

@BE-Webdesign

BE-Webdesign Oct 4, 2017
Author Contributor

Now I understand, thank you. Yes, we can do something like this, makes sense.

case 'HANDLE_METABOX_RELOAD':
return { ...state, [ action.location ]: { ...state[ action.location ], isUpdating: false, isDirty: false } };
case 'REQUEST_METABOX_UPDATE':
return { ...state, [ action.location ]: { ...state[ action.location ], isUpdating: true, isDirty: false } };

This comment has been minimized.

@aduth

aduth Oct 2, 2017
Member

Is it really fair to say that the post is not dirty at this point, since the save is still pending completion? How does this tie into the page dirty prompt behavior?

This comment has been minimized.

@BE-Webdesign

BE-Webdesign Oct 2, 2017
Author Contributor

Good point. If the post save is unsuccessful, this would be a problem. I will have to think about how to fix this.

This comment has been minimized.

@BE-Webdesign

BE-Webdesign Oct 4, 2017
Author Contributor

So granted this is confusing, but the request for metabox updating does not happen until the post has successfully saved, while that is updating, we want to keep the state not dirty. Visually it looks like this happens from when you hit Update/Publish, but that is not what is actually happening state wise. Potentially what should be done is to have an effect on REQUEST_METABOX_UPDATES that will keep the post save button in a state of updating?

@BE-Webdesign
Copy link
Contributor Author

@BE-Webdesign BE-Webdesign commented Oct 2, 2017

Thank you for the review everyone! I will get this shaped up either late tonight, or early tomorrow morning. @karmatosed I will also fix up the input fields! Meant to do that but it slipped through the cracks.

@pento
Copy link
Member

@pento pento commented Oct 18, 2017

Rebase needs to wait for #3052, because that's causing all sorts of fun bugs in this PR, too. :-)

Basic metabox support please see docs/metabox.md for details.
@BE-Webdesign BE-Webdesign force-pushed the support/metaboxes-rebase branch from c4ca564 to b623d2b Oct 18, 2017
@BE-Webdesign
Copy link
Contributor Author

@BE-Webdesign BE-Webdesign commented Oct 18, 2017

@pento, @mtias.

Should work now. This needs your blessing. Rebased the latest changes. With the recent restructuring in bdf94e6, there are probably some simplifications to be made in this, and I already made some, but for now this is probably fine.

@pento pento merged commit 02d4734 into master Oct 19, 2017
3 checks passed
3 checks passed
codecov/project 32.4% (-0.19%) compared to 15c515b
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@pento pento deleted the support/metaboxes-rebase branch Oct 19, 2017
@jasmussen
Copy link
Contributor

@jasmussen jasmussen commented Oct 19, 2017

💥💥💥💥💥💥💥💥💥💥💥💥💥💥

@youknowriad
Copy link
Contributor

@youknowriad youknowriad commented Oct 19, 2017

Huge kudos for @BE-Webdesign ❤️

@sc0ttkclark
Copy link

@sc0ttkclark sc0ttkclark commented Oct 19, 2017

Looking into playing with this for Pods integration now!

@BE-Webdesign
Copy link
Contributor Author

@BE-Webdesign BE-Webdesign commented Oct 19, 2017

Huge kudos for @BE-Webdesign ❤️

Team effort!

@ahmadawais
Copy link
Contributor

@ahmadawais ahmadawais commented Oct 20, 2017

Huge props @BE-Webdesign!

🔥🔥🔥🔥🔥🔥🔥🔥🔥🔥

$notice = false;
$form_extra = '';
if ( 'auto-draft' === $post->post_status ) {
if ( 'edit' === $action ) {

This comment has been minimized.

@aduth

aduth Oct 20, 2017
Member

$action is not assigned in this scope.

Notice: Undefined variable: action in /srv/www/editor/htdocs/wp-content/plugins/gutenberg/lib/meta-box-partial-page.php on line 345

This comment has been minimized.

@BE-Webdesign

BE-Webdesign Oct 20, 2017
Author Contributor

Woops.

This comment has been minimized.

@BE-Webdesign

BE-Webdesign Oct 20, 2017
Author Contributor

BE-Webdesign added a commit that referenced this pull request Oct 20, 2017
Fix minor PHP error. See
#2804 (comment)
@khromov
Copy link

@khromov khromov commented Oct 24, 2017

Not working for me in Firefox 56. The Extended settings box does not expand when clicked. Latest Chrome works though. Error log from Firefox:

screen shot 2017-10-24 at 13 34 08

@lkraav
Copy link

@lkraav lkraav commented Oct 24, 2017

Not working for me in Firefox 56. The Extended settings box does not expand when clicked. Latest Chrome works though. Error log from Firefox:

I can confirm both findings (FF57). Didn't even think to try on Chrome, I just assumed it was still half-baked/broken. But indeed, Chrome works. I would not recommend skipping testing on FF any longer, FF57 Quantum is a significant enough improvement that it will certainly gain market share.

@mtias
Copy link
Contributor

@mtias mtias commented Oct 24, 2017

Just released 1.5.1 to fix the FF issue—thought we had fixed this one before. Sorry about that.

@lkraav
Copy link

@lkraav lkraav commented Oct 31, 2017

Just released 1.5.1 to fix the FF issue—thought we had fixed this one before. Sorry about that.

@mtias I have 1.6.0 not showing Extended Settings on Firefox again. Same thing or...?

@lukecav
Copy link

@lukecav lukecav commented Nov 15, 2017

@lkraav

Which version of Gutenberg are you using 1.7.0?

Also which version of Mozilla Firefox?

@lkraav
Copy link

@lkraav lkraav commented Nov 15, 2017

@lukecav we can deprecate the previous comment, I just installed 1.7.0 today and will re-evaluate.

@Rahmon Rahmon mentioned this pull request Jan 30, 2018
3 of 3 tasks complete
@pyronaur
Copy link

@pyronaur pyronaur commented Sep 13, 2018

@BE-Webdesign
Hey Edwin,
An issue popped up recently in Gutenberg Ramp that's related to the Metabox patch you worked on.

I have a question regarding this bit:

// As late as possible, but before any logic that adds meta boxes.
add_action(
'plugins_loaded',
'gutenberg_trick_plugins_into_registering_meta_boxes'
);

Is priority 10 the real as late as possible priority? Could it be bumped up to at least 30 without breaking anything?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

You can’t perform that action at this time.