Framework: Merge editor and blocks modules into a single editor module #2795

youknowriad · 2017-09-26T12:11:05Z

Description

This PR merges the blocks and editor modules into a single editor module, this separation was "artificial" since Block API don't work without an editor. This is the first step towards #2761

Checklist:

move the content of thee blocks module into the editor module.
merge the two index.js files.
update @wordpress/blocks dependeency usage to internal imports
Fix loading the assets and the webpack config
Fix the tests
Update the documentation
Update the components' classnames
Alias the old wp.blocks.* calls to avoid BC

How Has This Been Tested?

Test that everything works, the editor loads, you're able to edit/save posts (quickly)

It would be great if we can merge this quickly to avoid multiple rebases

codecov · 2017-09-26T12:11:07Z

Codecov Report

Merging #2795 into master will decrease coverage by 0.14%.
The diff coverage is 2.7%.

@@            Coverage Diff            @@
##           master   #2795      +/-   ##
=========================================
- Coverage   33.95%   33.8%   -0.15%     
=========================================
  Files         191     192       +1     
  Lines        5690    5709      +19     
  Branches      997    1003       +6     
=========================================
- Hits         1932    1930       -2     
- Misses       3180    3195      +15     
- Partials      578     584       +6

Impacted Files	Coverage Δ
editor/block-api/paste/normalise-blocks.js	`96.87% <ø> (ø)`
editor/blocks/categories/data.js	`0% <ø> (ø)`
editor/block-alignment-toolbar/index.js	`33.33% <ø> (ø)`
editor/block-autocomplete/index.js	`100% <ø> (ø)`
editor/blocks/pullquote/index.js	`33.33% <ø> (ø)`
editor/inspector-controls/select-control/index.js	`0% <ø> (ø)`
editor/blocks/more/index.js	`22.22% <ø> (ø)`
editor/blocks/table/table-block.js	`8% <ø> (ø)`
editor/blocks/quote/index.js	`16.66% <ø> (ø)`
editor/blocks/gallery/block.js	`11.76% <ø> (ø)`
... and 94 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 0ab6abd...c2683df. Read the comment docs.

aduth · 2017-09-26T21:02:45Z

Fix the tests

There appear to be some failing tests yet. Restarting build did not appear to help.

youknowriad · 2017-09-26T21:22:07Z

@aduth Yeah, I'll take a look and I also remembered that I have to change all the classnames :)

mtias · 2017-09-27T12:53:25Z

Since this would break all existing 3rd party blocks, can we alias the global for a couple versions and maybe log a warning?

youknowriad · 2017-09-27T15:37:04Z

@mtias Good idea! I think we can't alias everything but we should through a warning in registerBlock only, what do you think?

aduth · 2017-09-28T01:16:02Z

Do we foresee moving from block registration occurring on a global editor instance to individual instance of an editor, and if so, should we consider those implications as that change too would likely incur similar breaking changes?

youknowriad · 2017-09-28T08:26:12Z

@aduth

Do we foresee moving from block registration occurring on a global editor instance to individual instance of an editor, and if so, should we consider those implications as that change too would likely incur similar breaking changes?

I don't think this change will be a breaking change (see my last PR exploring this #2182). The registration is still global, but the Editor do not use the globally registered blocks but a subset provided by a config (which could be provided by a parent block, a cpt config or anything else). It's also probably too soon to know exactly.

youknowriad · 2017-09-28T08:50:33Z

I added the aliased module, Let me know if you have a better wording for the warning.

youknowriad · 2017-09-29T10:43:52Z

Any other blocker here?

aduth · 2017-09-29T14:07:04Z

blocks/index.js

-export { default as UrlInputButton } from './url-input/button';
+import { registerBlockType as oldRegisterBlockType } from '@wordpress/editor';
+
+const wrapWithDeprecationWarning = ( func ) => function() {


Any specific reason we mix arrow and function syntaxes here? Could use arrow function in the latter case?

I wanted to use arguments but I can replace with ...args

args please 😃

aduth · 2017-09-29T14:07:10Z

blocks/index.js

+const wrapWithDeprecationWarning = ( func ) => function() {
+	console.warn( // eslint-disable-line no-console
+		'The "wp.blocks" module has been deprecated and replaced by "wp.editor". please update your usage.' +
+	' Example: wp.editor.registerBlock instead of wp.blocks.registerBlock'


This should be indented one more

aduth · 2017-09-29T14:08:23Z

blocks/index.js

+	return func( ...arguments );
+};
+
+export const registerBlockType = wrapWithDeprecationWarning( oldRegisterBlockType );


What about other exports?

#2795 (comment)

The idea was not to flood block authors with warnings. Everyone will use registerBlock.

aduth · 2017-09-29T14:12:48Z

editor/README.md

@@ -7,8 +7,391 @@ The logic flow concerning the editor includes: inferring a block representation

 The goal of the editor element is to let the user manipulate the content of their posts in a deterministic way—organized through the presence of blocks of content. Usually, in a declarative flow, the pieces that compose a post would be represented in a certain order and the machine would be able to generate an output view from it with the necessary UI controls. However, we don’t begin in WordPress with a representation of the state of the post that is conductive to this expression nor one that even has any knowledge of blocks because content is stored in a serialized way in a single field.

-Such a crucial step is handled by the grammar parsing which takes the serialized content of the post and infers an ordered block list using, preferably, syntax hints present in HTML comments. The editor is initialized with a state representation of the block nodes generated by the parsing of the raw content of a post element: `wp.blocks.parse( post.content.raw )`.
+Such a crucial step is handled by the grammar parsing which takes the serialized content of the post and infers an ordered block list using, preferably, syntax hints present in HTML comments. The editor is initialized with a state representation of the block nodes generated by the parsing of the raw content of a post element: `wp.editor.parse( post.content.raw )`.


Some of these API conversions don't translate too well. Is wp.editor.parse as clear as wp.blocks.parse in what it's doing? Maybe... do we need to be more specific with block terminology in the exported names? wp.editor.parseBlocks ?

I'm on the fence.

No strong opinion, and honestly, I'm not sure this will stay this way once we completely refactor the editor with the generic components approach.

We had the similar issues before, e.g. the method like wp.blocks.getCategories. We can iterate later.

aduth · 2017-09-29T14:14:42Z

docs/coding-guidelines.md

@@ -35,7 +35,7 @@ You may observe that these conventions adhere closely to the [BEM (Blocks, Eleme

 The build process will split SCSS from within the blocks library directory into two separate CSS files when Webpack runs.

-Styles placed in a `style.scss` file will be built into `blocks/build/style.css`, to load on the front end theme as well as in the editor. If you need additional styles specific to the block's display in the editor, add them to an `editor.scss`.


What would become our updated guideline for class name prefixing? I notice we're still using block- as prefix for classes in blocks (e.g. .blocks-gallery-image), despite the guideline currently recommending that anything in editor be prefixed as editor-

I understood it like this:

The guidelines apply for the application classnames but not the frontend classnames theme authors should be aware of.

aduth · 2017-09-29T14:18:21Z

editor/modes/visual-editor/test/inserter.js

@@ -9,6 +9,10 @@ import { shallow } from 'enzyme';
 import { VisualEditorInserter } from '../inserter';

 describe( 'VisualEditorInserter', () => {
+	beforeAll( () => {
+		require( '../../../library' );


Why do we need this now?

I guess it registers all block types.

aduth · 2017-09-29T14:19:57Z

editor/test/reducer.js

@@ -921,6 +917,7 @@ describe( 'state', () => {

 	describe( 'userData()', () => {
 		beforeAll( () => {
+			require( '../library' );


Same question as previous: We didn't need this previously? Or was it the case that because we imported from @wordpress/blocks we implicitly included the full library as well? 😬

The best thing to do is refactoring the test to not rely on these, but this PR is big enough :)

I like it that we need to explicitly register those blocks. It just isn't clear from the require statement. We can revisit later.

aduth · 2017-10-02T14:00:52Z

Some of the folder names lose their meaning when moved from blocks to editor, and we might consider renaming them for clarity.

For example:

editor/library -> editor/blocks
editor/api -> editor/block-api

In general, the editor folder has become rather disorganized now after these changes. I think this will be improved in future pull requests by pulling out post editing specific components. We could also consider introducing new folders to organize, maybe tying into #2761 as a component organization pattern. Since we've gone back and forth on this a few times in the past, it's worth being cautious, but it could help avoid blocks and block-api from becoming lost amongst the other components.

Edit: To be clear, aside from some of the simple renaming, I don't want to suggest we come up with organization strategies for this pull request. I've also been thinking about how to better organize state, and these can be done in subsequent pull requests.

gziolo · 2017-10-02T14:06:36Z

editor/library/quote/editor.scss

@@ -0,0 +1,4 @@
+.wp-block-quote.blocks-quote-style-2 > .editor-editable p {


.blocks-quote-style-2 shouldn't it start with editor now?

#2795 (comment)

Since these are also frontend classes too, I think we should have a separate rule for those because theme authors are likely to style them. We do not have a fixed rule for this yet, maybe wp-block__*, but this is a separate concern IMO

There are a few classes that start with .blocks-:

Please double check all of them should stay.

Yep, these should stay :)

Edit: maybe not the remove link, checking

The remove link is a special-case, it's included in a component used in save and edit but is shown only on edit (via a prop). I'm going to leave it as is for now, not clear answer here.

gziolo · 2017-10-02T14:13:54Z

webpack.config.js

@@ -104,21 +104,21 @@ const config = {
 			{
 				test: /style\.s?css$/,
 				include: [
-					/blocks/,
+					/editor\/library/,


Why not editor/blocks? 😃

Yes, that's what @aduth and @mtias aree suggesting too, updating

mtias · 2017-10-02T14:14:14Z

I was also going to suggest editor/library -> editor/blocks.

youknowriad · 2017-10-02T14:36:24Z

We could also consider introducing new folders to organize, maybe tying into #2761 as a component organization pattern.

@aduth Agreed, I think we need to group the editor components together in something like editor/components maybe

…ary` to `editor/blocks`

youknowriad · 2017-10-12T09:36:39Z

I still think this makes sense, but let's try to address #2761 in a different order.

youknowriad added the Framework Issues related to broader framework topics, especially as it relates to javascript label Sep 26, 2017

youknowriad self-assigned this Sep 26, 2017

youknowriad requested review from mtias and aduth September 26, 2017 12:11

youknowriad force-pushed the update/mergee-editor-and-blocks branch from 29071f6 to 21adb83 Compare September 26, 2017 12:28

youknowriad force-pushed the update/mergee-editor-and-blocks branch from 21adb83 to b0d8521 Compare September 27, 2017 09:26

youknowriad force-pushed the update/mergee-editor-and-blocks branch 2 times, most recently from c21d30e to 386a9a1 Compare September 29, 2017 10:42

aduth reviewed Sep 29, 2017

View reviewed changes

youknowriad force-pushed the update/mergee-editor-and-blocks branch 4 times, most recently from c5e7b9e to 6183ff7 Compare October 2, 2017 12:33

gziolo reviewed Oct 2, 2017

View reviewed changes

youknowriad mentioned this pull request Oct 3, 2017

Add reusable blocks #2659

Closed

gziolo mentioned this pull request Oct 3, 2017

Blocks: Consider moving UI components out of /blocks folder #2788

Closed

youknowriad force-pushed the update/mergee-editor-and-blocks branch from 53f30fb to e116c0a Compare October 3, 2017 10:53

youknowriad added 4 commits October 4, 2017 10:14

Framework: Merge editor and blocks modules into a single editor module

a5bbb8a

Framework: rename blocks- classNames to editor- since we moved the files

75767a6

Framework: Adding a legacy blocks package to avoid Breaking Changes

66b1264

Framework: rename editor/api to editor/block-api and `editor/libr…

c2683df

…ary` to `editor/blocks`

youknowriad force-pushed the update/mergee-editor-and-blocks branch from e116c0a to c2683df Compare October 4, 2017 09:16

youknowriad closed this Oct 12, 2017

noisysocks mentioned this pull request Dec 6, 2017

Reusable Blocks: Add reusable blocks UI #3378

Merged

gziolo deleted the update/mergee-editor-and-blocks branch May 7, 2018 11:15

		@@ -35,7 +35,7 @@ You may observe that these conventions adhere closely to the [BEM (Blocks, Eleme

		The build process will split SCSS from within the blocks library directory into two separate CSS files when Webpack runs.

		Styles placed in a `style.scss` file will be built into `blocks/build/style.css`, to load on the front end theme as well as in the editor. If you need additional styles specific to the block's display in the editor, add them to an `editor.scss`.

		@@ -0,0 +1,4 @@
		.wp-block-quote.blocks-quote-style-2 > .editor-editable p {

Framework: Merge editor and blocks modules into a single editor module #2795

Framework: Merge editor and blocks modules into a single editor module #2795

Conversation

youknowriad commented Sep 26, 2017 • edited

Description

Checklist:

How Has This Been Tested?

codecov bot commented Sep 26, 2017 • edited

Codecov Report

aduth commented Sep 26, 2017

youknowriad commented Sep 26, 2017

mtias commented Sep 27, 2017

youknowriad commented Sep 27, 2017

aduth commented Sep 28, 2017

youknowriad commented Sep 28, 2017 • edited

youknowriad commented Sep 28, 2017

youknowriad commented Sep 29, 2017

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 Sep 29, 2017 • edited

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

aduth commented Oct 2, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

youknowriad Oct 2, 2017 • edited

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

mtias commented Oct 2, 2017

youknowriad commented Oct 2, 2017

youknowriad commented Oct 12, 2017

youknowriad commented Sep 26, 2017 •

edited

codecov bot commented Sep 26, 2017 •

edited

youknowriad commented Sep 28, 2017 •

edited

youknowriad Sep 29, 2017 •

edited

aduth commented Oct 2, 2017 •

edited

youknowriad Oct 2, 2017 •

edited