Framework: Extract "edit-post" to its own module #4661

youknowriad · 2018-01-24T17:20:01Z

This PR extracts the edit-post module to its own module. This step is helpful to be able to experiment with an "edit-template" later.

The new module has its own store handling layout state.
Extract some more reusable components in editor for use in edit-post (BlockSelectionClearer, InserterWithShortcuts...)
The "Show block inspector" button proved to be a challenge, because it's layout related. this is fixed by providing a way to extend the Block's menu from outside the editor module using a prop.
Renamed classNames for the components moved to the new module.

Testing instructions

This may introduce some unnoticed regressions, heavily testing this PR is necessary.

aduth

I agree with general direction that this helps us toward consolidating editor and blocks directory as also noted in #2761. There's a few open questions remaining on state responsible for editor vs. edit-post (e.g. currentPost remains in editor state) that we could consider refactoring further in subsequent pull requests.

The addition of storeKey to each connected component is quite verbose. I think we should consider either a connect custom to each top-level module which serves as a wrapper to React-Redux's connect passing the storeKey automatically. Alternatively, we could seek to enhance our own query HoC to support all of the same functionality as connect and then it would be not necessary.

Generally difficult to review some of the more granular changes, as while some files are moved verbatim, others include revisions.

aduth · 2018-01-25T12:54:46Z

editor/components/block-selection-clearer/index.js

+				onMouseDown={ this.onClick }
+				onTouchStart={ this.onClick }
+				ref={ this.bindContainer }
+				{ ...omit( props, 'clearSelectedBlock' ) }


FWIW you wouldn't need to explicitly pick children to render, since it's already a part of this.props that would be passed.

aduth · 2018-01-25T12:55:34Z

editor/components/block-settings-menu/index.js

 				// Should this just use a DropdownMenu instead of a DropDown ?
 				<NavigableMenu className="editor-block-settings-menu__content">
-					<BlockInspectorButton onClick={ onClose } />
+					{ renderBlockMenu( { onClose } ) }


The prop name renderBlockMenu implies that it replaces the entirety of the menu rendering, not adding new options to the top. Would we ever need to replace all rendering? Append to the end? Is this somewhere we want to leverage hooks instead?

aduth · 2018-01-25T12:57:15Z

editor/components/post-schedule/check.js

+ */
+import { getCurrentPostType } from '../../store/selectors';
+
+export function PostScheduleCheck( { user, children } ) {


To prior comments made at #4662, I could imagine an HoC:

ifUserCan( 'publish_posts' )

aduth · 2018-01-25T13:02:55Z

lib/client-assets.php

+	wp_register_script(
+		'wp-edit-post',
+		gutenberg_url( 'edit-post/build/index.js' ),
+		array( 'wp-element', 'wp-components', 'wp-editor', 'wp-i18n', 'wp-date', 'wp-utils' ),


edit-post/index.js has a reference to jQuery and heartbeat, so this should be made an explicit dependency.

Heartbeat is no longer used as a dependency in wp-editor.

aduth · 2018-01-25T13:05:32Z

webpack.config.js

 	'i18n',
 	'utils',
 	'data',
+	'edit-post',


To be able to export this as wp.editPost we should consider mapping packages using Lodash's _.camelCase.

Might not be possible to configure webpack to do so https://webpack.js.org/configuration/output/#output-library

aduth · 2018-01-25T13:07:13Z

package.json

 	},
 	"jest": {
 		"collectCoverageFrom": [
-			"(blocks|components|date|editor|element|i18n|data|utils)/**/*.js"


We also include a custom ESLint rule for each top-level directory in .eslintrc.json:

gutenberg/.eslintrc.json

Lines 95 to 125 in e694f6a

"selector": "ImportDeclaration[source.value=/^blocks$/]",

"message": "Use @wordpress/blocks as import path instead."

},

{

"selector": "ImportDeclaration[source.value=/^components$/]",

"message": "Use @wordpress/components as import path instead."

},

{

"selector": "ImportDeclaration[source.value=/^date$/]",

"message": "Use @wordpress/date as import path instead."

},

{

"selector": "ImportDeclaration[source.value=/^editor$/]",

"message": "Use @wordpress/editor as import path instead."

},

{

"selector": "ImportDeclaration[source.value=/^element$/]",

"message": "Use @wordpress/element as import path instead."

},

{

"selector": "ImportDeclaration[source.value=/^i18n$/]",

"message": "Use @wordpress/i18n as import path instead."

},

{

"selector": "ImportDeclaration[source.value=/^data$/]",

"message": "Use @wordpress/data as import path instead."

},

{

"selector": "ImportDeclaration[source.value=/^utils$/]",

"message": "Use @wordpress/utils as import path instead."

},

youknowriad · 2018-01-25T15:11:25Z

Alternatively, we could seek to enhance our own query HoC to support all of the same functionality as connect and then it would be not necessary.

If we're fine exposing all selectors to third-party modules, we can get rid of the connect usage.

--

I addressed most of the feedback. Thanks @aduth

aduth · 2018-01-28T11:01:50Z

editor/components/block-list/block.js

 				<BlockDropZone index={ order } />
 				{ ( showUI || isHovered ) && <BlockMover uids={ [ block.uid ] } /> }
-				{ ( showUI || isHovered ) && <BlockSettingsMenu uids={ [ block.uid ] } /> }
+				{ ( showUI || isHovered ) && <BlockSettingsMenu uids={ [ block.uid ] } renderBlockMenu={ renderBlockMenu } /> }


This change with renderBlockMenu broke the multi-select controls dropdown:

Create a multi-selection range

Open "More Options" menu

Good catch, we should cherry-pick the fix from the templates branch to its own PR

aduth · 2018-01-29T09:30:02Z

Nitpick: To the naming of this module, it stands out to me as inconsistent with our general naming, not only because it's intended as a superset of functionality for the base editor (but naming does not imply this), but also that other modules generally tend to be nouns (editor, blocks, components), whereas edit-post is more an action/verb.

My suggested alternative would be post-editor. What do you think?

aduth · 2018-01-29T09:30:51Z

Also, we should plan to fix the assignment of the global to avoid wp[ 'edit-post' ] and instead as wp.editPost. I agree it wasn't a blocker for this pull request, but I don't think we should settle on the non-camelCased global property.

youknowriad · 2018-01-29T09:53:59Z

cc @mtias for the name of the module.

About the global, this might mean we need to find another way to assign these variables (instead of relying on the library config of webpack)

aduth · 2018-01-30T20:03:42Z

editor/components/block-list/index.js


 		return (
-			<div>
+			<BlockSelectionClearer>


Why was this changed from a div to BlockSelectionClearer ?

Because this was handled in the parent component by using a ref to this div there. I just made this component generic and used it in both the VisualEditor component and the inner component BlockList

jasmussen · 2018-01-31T09:23:10Z

I think this PR regressed the color scheme styling of the tabs in the sidebar:

As you can see from the Publish button, it's supposed to be red, like other tab buttons:

It should be a fairly easy fix, renaming some CSS classes in edit-post/assets/stylesheets/_admin-schemes.scss.

Do you want to do it, @youknowriad, or would you like me to do it? I have time if you're busy.

Changed in #4661, intention not clear.

These data has been moved to edit-post store as per #4661

youknowriad and others added 3 commits January 24, 2018 17:34

Framework: Extract edit-post to its own module

ab294d1

Fixing the unit tests

53dc3d1

Rename 'editor-*' classes to 'edit-post-*' when applicable

1c97fe8

youknowriad force-pushed the update/extract-edit-post-v2 branch from b2572b6 to 1c97fe8 Compare January 24, 2018 17:36

Fix e2e tests

a5302b0

youknowriad force-pushed the update/extract-edit-post-v2 branch from e59a0cd to a5302b0 Compare January 25, 2018 11:57

youknowriad added 2 commits January 25, 2018 12:02

Editor with shortcuts: Fix the classnames

5f2ec6c

Fix Block Inspector Button classname

831f1eb

aduth reviewed Jan 25, 2018

View reviewed changes

youknowriad added 2 commits January 25, 2018 13:28

Changes per review

36b293b

update renderBlockMenu API

b8ad763

youknowriad merged commit 8c5678c into master Jan 25, 2018

youknowriad deleted the update/extract-edit-post-v2 branch January 25, 2018 18:30

aduth reviewed Jan 28, 2018

View reviewed changes

youknowriad mentioned this pull request Jan 29, 2018

Block Menu: Fix the default render function #4729

Merged

aduth reviewed Jan 30, 2018

View reviewed changes

aduth added a commit that referenced this pull request Jan 31, 2018

Block List: Revert block list selection clearer to div

4d0564c

Changed in #4661, intention not clear.

aduth mentioned this pull request Jan 31, 2018

Post Editor: Fix lingering visual editor classes #4781

Merged

This was referenced Feb 8, 2018

Build: Consolidate module build JS, CSS, map inclusion #4957

Merged

Build: Assign edit-post global as wp.editPost #4966

Merged

aduth mentioned this pull request Feb 15, 2018

Expose getEditedPostAttribute selector #5003

Merged

3 tasks

oandregal added a commit that referenced this pull request Sep 14, 2018

Remove outdated docs

6dbf2ea

These data has been moved to edit-post store as per #4661

oandregal added a commit that referenced this pull request Sep 17, 2018

Remove outdated docs

d1c69c5

These data has been moved to edit-post store as per #4661

	"selector": "ImportDeclaration[source.value=/^blocks$/]",
	"message": "Use @wordpress/blocks as import path instead."
	},
	{
	"selector": "ImportDeclaration[source.value=/^components$/]",
	"message": "Use @wordpress/components as import path instead."
	},
	{
	"selector": "ImportDeclaration[source.value=/^date$/]",
	"message": "Use @wordpress/date as import path instead."
	},
	{
	"selector": "ImportDeclaration[source.value=/^editor$/]",
	"message": "Use @wordpress/editor as import path instead."
	},
	{
	"selector": "ImportDeclaration[source.value=/^element$/]",
	"message": "Use @wordpress/element as import path instead."
	},
	{
	"selector": "ImportDeclaration[source.value=/^i18n$/]",
	"message": "Use @wordpress/i18n as import path instead."
	},
	{
	"selector": "ImportDeclaration[source.value=/^data$/]",
	"message": "Use @wordpress/data as import path instead."
	},
	{
	"selector": "ImportDeclaration[source.value=/^utils$/]",
	"message": "Use @wordpress/utils as import path instead."
	},

Framework: Extract "edit-post" to its own module #4661

Framework: Extract "edit-post" to its own module #4661

Uh oh!

Conversation

youknowriad commented Jan 24, 2018

Uh oh!

aduth 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 Jan 25, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aduth commented Jan 29, 2018

Uh oh!

aduth commented Jan 29, 2018

Uh oh!

youknowriad commented Jan 29, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasmussen commented Jan 31, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants