Framework: Merge editor and blocks modules into a single editor module by youknowriad · Pull Request #2795 · WordPress/gutenberg

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

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

This should be indented one more

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

blocks/index.js

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

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

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

Why do we need this now?

I guess it registers all block types.

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

editor/test/reducer.js

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 aduth and mtias 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

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

Conversation

youknowriad commented Sep 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Checklist:

How Has This Been Tested?

Uh oh!

codecov bot commented Sep 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

aduth commented Sep 26, 2017

Uh oh!

youknowriad commented Sep 26, 2017

Uh oh!

mtias commented Sep 27, 2017

Uh oh!

youknowriad commented Sep 27, 2017

Uh oh!

aduth commented Sep 28, 2017

Uh oh!

youknowriad commented Sep 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

youknowriad commented Sep 28, 2017

Uh oh!

youknowriad commented Sep 29, 2017

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 Sep 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

aduth commented Oct 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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 Oct 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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

youknowriad commented Sep 26, 2017 •

edited

Loading

codecov bot commented Sep 26, 2017 •

edited

Loading

youknowriad commented Sep 28, 2017 •

edited

Loading

youknowriad Sep 29, 2017 •

edited

Loading

aduth commented Oct 2, 2017 •

edited

Loading

youknowriad Oct 2, 2017 •

edited

Loading