MetaBoxes: Moving meta boxes out of the generic editor module #5175

youknowriad · 2018-02-21T12:18:45Z

The editor module is a generic module meant to be used to build any editing interface (template editor, post editor, p2 editor...) it shouldn't be concerned about the meta boxes which are post editor specific.

I'm pretty happy with this PR, I was able to extract all the meta box logic out of the editor module by doing:

Moving the components to the edit-post module
Moving the metabox state to the edit-post module
Hooking into the PostSavedStatus component to show the isLoading state when the meta boxes are loading and to always consider the post dirty if there are meta boxes on the page
Triggering meta boxes saving by watching isSavingPost selector. When the post was being saved and it's not anymore, trigger the meta boxes save behavior.

Testing instructions

Add some metaboxes
The "save" link should always be visible
When you save a post, the meta boxes are saved as well
When the meta boxes are saving, the top bar shows a loading state.

youknowriad · 2018-02-21T12:19:34Z

edit-post/components/layout/index.js

@@ -52,6 +53,7 @@ function Layout( {
 		<div className={ className }>
 			<DocumentTitle />
 			<UnsavedChangesWarning />
+			<MetaBoxesUnsavedWarning />


The logic to prevent from leaving the page needed a separate component for preventing leaving the page when the meta boxes are dirty.

youknowriad · 2018-02-21T12:22:06Z

The changes are not so easy to understand but basically, it's just moving pieces aside what I mention in the PR description.

youknowriad · 2018-02-21T12:27:31Z

Also with this change, I considered building a separate JavaScript module to handle the meta boxes, the edit-post could include depend on the module and any other WPAdmin screen having metaboxes could use it to add meta-boxes support.

If you think I should go this way, I can update th PR to do so.

aduth

Largely looks good and works well 👍

aduth · 2018-02-23T04:24:11Z

editor/components/post-saved-state/index.js

 		post: getCurrentPost( state ),
 		isNew: isEditedPostNew( state ),
 		isPublished: isCurrentPostPublished( state ),
-		isDirty: isEditedPostDirty( state ),
-		isSaving: isSavingPost( state ),
+		isDirty: isEditedPostDirty( state ) || forceIsDirty,


Minor: In this case the selector implementation is trivial, but caught my attention to have the function call as the first part of the condition, as if we expect forceIsDirty to be a simple truthy check, we could put it first and take advantage of short-circuit evaluation.

aduth · 2018-02-23T04:27:52Z

edit-post/store/middlewares.js

@@ -17,7 +18,7 @@ import { mobileMiddleware } from '../utils/mobile';
 */
 function applyMiddlewares( store ) {
 	const middlewares = [
-		mobileMiddleware,


I guess this just never existed and did nothing?

(Note: Will conflict with #5206)

I think it was removed in the plugin API sidebar but the files was kept

aduth · 2018-02-23T04:31:23Z

edit-post/components/meta-boxes/unsaved-warning.js

+
+class MetaBoxesUnsavedWarning extends Component {
+	/**
+	 * @inheritdoc


Do we care for these JSDoc? I'm guessing from copy paste of underlying UnsavedChangesWarning.

Speaking of, it would be nice it this wasn't so non-DRY 😬

Yep, I was wondering if I should create another component or provide an extensibility APi or a prop or something for the other one, I thought this one is simpler to understand, but I can update to introduce a callaback to enhance the isDirty check.

aduth · 2018-02-23T04:31:56Z

edit-post/components/meta-boxes/unsaved-warning.js

+	}
+}
+
+export default connect(


With #5137, maybe update to use withSelect ?

aduth · 2018-02-23T04:32:57Z

edit-post/components/meta-boxes/meta-boxes-area/index.js

@@ -86,4 +86,9 @@ function mapStateToProps( state ) {
 	};
 }

-export default connect( mapStateToProps )( MetaBoxesArea );
+export default connect(


With #5137, maybe update to use withSelect ? Might require registering more/all selectors for edit-post (also done in #5206).

I think we should do a separate PR and move all of these at once :)

aduth · 2018-02-23T04:34:39Z

edit-post/components/header/index.js

@@ -49,7 +53,10 @@ function Header( {
 			<HeaderToolbar />
 			{ ! isPublishSidebarOpen && (
 				<div className="edit-post-header__settings">
-					<PostSavedState />
+					<PostSavedState
+						forceIsDirty={ hasActiveMetaboxes }


Minor: Curious if we need to distinguish this from the presentational isDirty prop, or if we ought to just pass this as isDirty and defer to use it in the mapStateToProps.

you mean shortcut the isDirty selector entirely right? which would also mean add a check for post dirtiness here. I don't feel strongly either way.

aduth · 2018-02-23T04:37:06Z

editor/store/selectors.js

@@ -966,7 +920,8 @@ export function isBlockInsertionPointVisible( state ) {
 * @return {boolean} Whether post is being saved.
 */
 export function isSavingPost( state ) {
-	return state.saving.requesting || isSavingMetaBoxes( state );
+	// TODO: filter


In what way or reason are you suggesting "filter" ?

Edit: I guess so we wouldn't have to pass forceIsDirty and forceIsSaving as props?

Yes, actually I added these comments before adding the props, I think I can remove them right now :)

youknowriad added the [Feature] Meta Boxes A draggable box shown on the post editing screen label Feb 21, 2018

youknowriad self-assigned this Feb 21, 2018

youknowriad requested review from mtias, gziolo and aduth February 21, 2018 12:18

youknowriad commented Feb 21, 2018

View reviewed changes

youknowriad mentioned this pull request Feb 21, 2018

Add check for window.postboxes #4816

Closed

3 tasks

youknowriad force-pushed the refactor/move-meta-box-out-of-editor branch 2 times, most recently from 4effe6b to 4ea8dd0 Compare February 21, 2018 14:40

aduth approved these changes Feb 23, 2018

View reviewed changes

youknowriad added 2 commits February 23, 2018 09:19

MetaBoxes: Moving meta boxes out of the generic editor module

b5fa8a4

add a forceIsDirty callback for the UnsavedChangesWarning component

35cdb5c

youknowriad force-pushed the refactor/move-meta-box-out-of-editor branch from 4ea8dd0 to 35cdb5c Compare February 23, 2018 08:33

youknowriad merged commit ee3a61c into master Feb 23, 2018

youknowriad deleted the refactor/move-meta-box-out-of-editor branch February 23, 2018 08:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MetaBoxes: Moving meta boxes out of the generic editor module #5175

MetaBoxes: Moving meta boxes out of the generic editor module #5175

youknowriad commented Feb 21, 2018

youknowriad Feb 21, 2018

youknowriad commented Feb 21, 2018 •

edited

Loading

youknowriad commented Feb 21, 2018

aduth left a comment

aduth Feb 23, 2018

aduth Feb 23, 2018

youknowriad Feb 23, 2018

aduth Feb 23, 2018

youknowriad Feb 23, 2018

aduth Feb 23, 2018

aduth Feb 23, 2018

youknowriad Feb 23, 2018

aduth Feb 23, 2018

youknowriad Feb 23, 2018

aduth Feb 23, 2018

youknowriad Feb 23, 2018

MetaBoxes: Moving meta boxes out of the generic editor module #5175

MetaBoxes: Moving meta boxes out of the generic editor module #5175

Conversation

youknowriad commented Feb 21, 2018

Choose a reason for hiding this comment

youknowriad commented Feb 21, 2018 • edited Loading

youknowriad commented Feb 21, 2018

aduth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

youknowriad commented Feb 21, 2018 •

edited

Loading