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

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 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';
Copy link
Contributor

Choose a reason for hiding this comment

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

Forgot a comment?

@jasmussen
Copy link
Contributor

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

@jasmussen I tested using ACF which seems to work properly

@youknowriad
Copy link
Contributor

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 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

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

Choose a reason for hiding this comment

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

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 ) ) );
Copy link
Contributor

@mcsf mcsf Sep 27, 2017

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup, definitely.

);
};

const Metabox = ( props ) => renderMetabox( props );
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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;
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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;
} }
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, saves an extra function creation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 );
Copy link
Contributor

Choose a reason for hiding this comment

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

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 ).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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 );
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

    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 );
    }

😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks good.

Copy link
Contributor Author

@BE-Webdesign BE-Webdesign Sep 29, 2017

Choose a reason for hiding this comment

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

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,
} );
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

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

@BE-Webdesign
Copy link
Contributor Author

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

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

@jasmussen
Copy link
Contributor

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

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

Choose a reason for hiding this comment

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

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 [
Copy link
Member

Choose a reason for hiding this comment

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

Why is this switched to an array?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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';
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 {
Copy link
Member

Choose a reason for hiding this comment

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

We prefer is-modifier classes like is-updating.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, no problem, will change..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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' );
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good.

docs/metabox.md Outdated
# 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,
Copy link
Member

Choose a reason for hiding this comment

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

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

docs/metabox.md Outdated

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
Copy link
Member

Choose a reason for hiding this comment

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

Typo: "metaboxe"

docs/metabox.md Outdated
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
Copy link
Member

Choose a reason for hiding this comment

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

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..."

docs/metabox.md Outdated

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`.
Copy link
Member

Choose a reason for hiding this comment

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

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 );

?>
Copy link
Member

Choose a reason for hiding this comment

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

Minor: Ending ?> can be omitted

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will get rid of it.


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

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 }
Copy link
Member

Choose a reason for hiding this comment

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

Is this class used?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 ) ) );
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure what you mean exactly.

Copy link
Contributor

Choose a reason for hiding this comment

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

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 } );

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 } };
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

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.

@BE-Webdesign
Copy link
Contributor Author

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?

Will probably entirely eliminate the Observer. I will finick around with Yoast at some point today to see what can be done.

editor/index.js Outdated
render(
<EditorProvider settings={ settings } post={ post }>
const provider = render(
<EditorProvider settings={ settings } post={ post } target={ target }>
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess padding the target is useless here? (probably a rebase thing)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I wasn't sure what happened in the branch, I will have to go look at it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed it, sorry about that 🙃

@pento
Copy link
Member

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
Copy link
Contributor Author

@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
@pento pento deleted the support/metaboxes-rebase branch October 19, 2017 10:41
@jasmussen
Copy link
Contributor

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

@youknowriad
Copy link
Contributor

Huge kudos for @BE-Webdesign ❤️

@sc0ttkclark
Copy link

Looking into playing with this for Pods integration now!

@BE-Webdesign
Copy link
Contributor Author

Huge kudos for @BE-Webdesign ❤️

Team effort!

@ahmadawais
Copy link
Contributor

Huge props @BE-Webdesign!

🔥🔥🔥🔥🔥🔥🔥🔥🔥🔥

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

Choose a reason for hiding this comment

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

$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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Woops.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@khromov
Copy link

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 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
Member

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 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 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 commented Nov 15, 2017

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

@pyronaur
Copy link
Contributor

@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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet