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

Move the Classic block to the packages #10397

Conversation

youknowriad
Copy link
Contributor

In order to merge Gutenberg into Core easily, all the Gutenberg JavaScript Core must be shared a reusable npm package. This PR moves the freeform block to the packages folder.

The idea is to test whether the wp.oldEditor global (provided by WP) is available or not before registering the block. An alternative proposed by @gziolo is to move this block to edit-post module. I think both alternatives are good options. We'll probably have to revisit at some point either way so I just went with the simplest one for noow.

This also means the block library is fully available as a package. (so a bunch of files removed etc...)

Testing instructions

Make sure you can use the classic block properly.

@youknowriad youknowriad added the npm Packages Related to npm packages label Oct 8, 2018
@youknowriad youknowriad self-assigned this Oct 8, 2018
@youknowriad youknowriad requested review from gziolo and a team October 8, 2018 08:48
@ellatrix
Copy link
Member

ellatrix commented Oct 8, 2018

Should the folders be renamed to classic instead of freeform?

@tofumatt
Copy link
Member

tofumatt commented Oct 8, 2018

Yes; the "freeform" name is quite confusing! 😅

@youknowriad
Copy link
Contributor Author

@tofumatt @iseulde The reason it's kept as "classic" is because we can't change the name of the block at the moment. I can rename the folder to "classic" but it feels like we'd create an inconsistency between block names and folders which all other blocks follow.

@gziolo
Copy link
Member

gziolo commented Oct 8, 2018

The reason it's kept as "classic" is because we can't change the name of the block at the moment. I can rename the folder to "classic" but it feels like we'd create an inconsistency between block names and folders which all other blocks follow.

It's called Classic in the UI:

screen shot 2018-10-08 at 17 12 19

I don't like exceptions, but in this case, it is only an internal name. We refer to it as classic every time we chat. I would vote to rename the folder and the variable assigned with import and leave the rest as is.

Sidenote: when you search for freeform in the inserter it finds nothing.

Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

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

This looks great. We could discuss whether it should land in edit-post but this makes all the things easier for the time being and removes a lot of dead code. I would still consider renaming the folder from freeform to classic.

@@ -44,6 +45,9 @@ import * as textColumns from './text-columns';
import * as verse from './verse';
import * as video from './video';

// The freeform block can't be moved to the "npm" packages folder because it requires the wp.oldEditor global.
Copy link
Member

Choose a reason for hiding this comment

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

This comment is no longer relevant, I guess 😅

registerBlockType( name, settings );
} );

setDefaultBlockName( paragraph.name );
if ( window.wp && window.wp.oldEditor ) {
Copy link
Member

Choose a reason for hiding this comment

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

We could register Classic block in here to avoid conditional in other parts.

if ( window.wp && window.wp.oldEditor ) {
    registerBlockType( freeform.name, freeform.settings );
    setUnknownTypeHandlerName( freeform.name );
}

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 wasn't sure about the implication of the order of the block's registration, I know it has some importance in the transforms priority.

Copy link
Member

Choose a reason for hiding this comment

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

I don't think it is important in this case, but I'm fine with leaving it as it is.

@gziolo gziolo added this to the 4.0 milestone Oct 8, 2018
@gziolo
Copy link
Member

gziolo commented Oct 8, 2018

We probably still need to add a changelog entry because technically speaking, you still can enable Classic block by defining window.wp.oldEditor global variable. It would mean version bump is minor because it adds new feature.

@youknowriad
Copy link
Contributor Author

I'll wait for a review of #10396 to merge this one as it's targetting that branch.

@@ -7,6 +7,7 @@
@import "./cover-image/editor.scss";
@import "./embed/editor.scss";
@import "./file/editor.scss";
@import "./freeform/editor.scss";
Copy link
Member

Choose a reason for hiding this comment

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

this should be updated, too

@youknowriad youknowriad merged commit c303621 into update/move-html-blokc-to-packages Oct 9, 2018
@youknowriad youknowriad deleted the update/move-freeform-block-to-packages branch October 9, 2018 07:12
youknowriad added a commit that referenced this pull request Oct 9, 2018
* Move the HTML block into the blocks library package

* Move the Classic block to the packages

* Move the Classic block to the packages
@talldan
Copy link
Contributor

talldan commented Oct 10, 2018

Hey @youknowriad - I’m seeing a warning in latest master:

Warning: filemtime(): stat failed for /var/www/html/wp-content/plugins/gutenberg/build/block-library/edit-blocks.css in /var/www/html/wp-content/plugins/gutenberg/lib/client-assets.php on line 741

It's still there after I cleaned the repo using git clean -dfx. I noticed some references to edit-blocks.css in this file, so I imagine there's a good chance it's related.

@gziolo
Copy link
Member

gziolo commented Oct 10, 2018

@talldan, looking into it.

@gziolo
Copy link
Member

gziolo commented Oct 10, 2018

I can't reproduce:

screen shot 2018-10-10 at 10 43 31

All 3 styles for block-library load properly. Also PHP code doesn't reference the file you shared:

wp_register_style(
		'wp-edit-blocks',
		gutenberg_url( 'build/block-library/editor.css' ),
		array(
			'wp-components',
			'wp-editor',
			// Always include visual styles so the editor never appears broken.
			'wp-block-library-theme',
		),
		filemtime( gutenberg_dir_path() . 'build/block-library/editor.css' )
	);

@talldan
Copy link
Contributor

talldan commented Oct 10, 2018

Hmm, unusual. Restarted my docker env and it's fine now. Thanks for looking into it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
npm Packages Related to npm packages
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants