Update for latest gutenberg compatibility #23

ajitbohra · 2018-05-31T15:00:27Z

Fixes console errors, warnings & outdated code.

Also, addresses following issues
Fixes #31
Fixes #30
Fixes #29
Fixes #25
Fixes #22
Fixes #21

Soean · 2018-05-31T15:30:10Z

01-basic/block.js

@@ -17,25 +17,25 @@
 	};

 	blocks.registerBlockType( 'gutenberg-examples/example-01-basic', {
-		title: __( 'Example: Basic', 'gutenberg-examples' ),


Why are you removing the textdomain?

Not all examples have text domain and currently it throws console error as we are not loading translations.

ajitbohra · 2018-06-04T19:25:30Z

@Soean textdomain added to all examples for consistency and setLocaleData to prevent console error.

Soean · 2018-06-04T19:27:58Z

Perfect, these examples should be best practices, so we should add i18n. Thanks :-)

aduth

Reviewing in passing: There's some coding standards updates which need to be made here around whitespace.

https://make.wordpress.org/core/handbook/best-practices/coding-standards/javascript/

aduth · 2018-07-09T13:12:27Z

01-basic/block.js

@@ -5,8 +5,8 @@
 *
 * Using inline styles - no external stylesheet needed.  Not recommended
 * because all of these styles will appear in `post_content`.
- */
-( function( blocks, i18n, element ) {
+ */ 


Excess whitespace at end of line.

aduth · 2018-07-09T13:12:42Z

01-basic/block.js

- */
-( function( blocks, i18n, element ) {
+ */ 
+(function (blocks, i18n, element) {


Shouldn't be space after function. Should be space before function.

aduth · 2018-07-09T13:12:52Z

01-basic/block.js

 			return el(
 				'p',
 				{ style: blockStyle },
 				'Hello World, step 1 (from the frontend).'
 			);
 		},
-	} );
-} )(
+	});


This spacing shouldn't be collapsed.

ajitbohra · 2018-07-09T18:16:15Z

@aduth thanks for the review coding standards fixed.

Is it good to merge?

aduth

We really need ESLint on this repository 😬

aduth · 2018-07-12T20:51:59Z

01-basic/block.js

-	} );
-} )(
+		}
+	});


Still whitespace issue here; space after }.

aduth · 2018-07-12T20:52:03Z

02-stylesheets/block.js

-	} );
-} )(
+		}
+	});


Still whitespace issue here; space after }.

aduth · 2018-07-12T20:52:38Z

03-editable/block.js

-	var children = blocks.source.children;
+	var RichText = editor.RichText;
+
+	wp.i18n.setLocaleData({ '': {} }, 'gutenberg-examples' );


Still whitespace issue here; space after setLocaleData(.

aduth · 2018-07-12T20:52:57Z

03-editable/block.js

@@ -19,36 +20,38 @@
 			content: {
 				type: 'array',
 				source: 'children',
-				selector: 'p',
-			},


Trailing commas are desirable (enforced in Gutenberg by ESLint)

aduth · 2018-07-12T20:53:09Z

03-editable/block.js

+				tagName: 'p', value: props.attributes.content
+			});
+		}
+	});


Still whitespace issue here; space after }.

aduth · 2018-07-12T20:53:26Z

05-recipe-card-esnext/block.js

-	edit: props => {
-		const focusedEditable = props.focus ? props.focus.editable || 'title' : null;
-		const attributes = props.attributes;
+	edit: (props) => {


Still whitespace issue here; space between parentheses.

iandunn · 2018-07-13T15:13:44Z

01-basic-esnext/block.js

@@ -7,8 +7,10 @@ const blockStyle = {
 	padding: '20px',
 };

+setLocaleData( { '': {} }, 'gutenberg-examples' );


Does this need to include the localized strings and current locale via gutenberg_get_jed_locale_data() ?

For example: https://github.com/swissspidy/gutenberg-i18n-block/blob/c2c59155f1dbbc957fa1531e42a4e4a490555253/gutenberg-i18n-block.php#L78

Related #27
cc @swissspidy

If so, then I think this approach might be a more readable, because it separates the data from the code:

// foo.php wp_add_inline_script( 'syntaxhighlighter-blocks', sprintf( ' var syntaxHighlighterData = { localeData: %s };', json_encode( gutenberg_get_jed_locale_data( 'syntaxhighlighter' ) ), ), 'before' );

// foo.js setLocaleData( syntaxHighlighterData.localeData, 'syntaxhighlighter' );

yes, you are right also this changes will help in understanding how to add localization to blocks.
will update all examples to properly implement localization.

Thank you

ajitbohra · 2018-07-13T18:03:01Z

@aduth yes it would be good to add eslint will update to include one. seems like my eslint/prettier is messing with those spaces.

ajitbohra · 2018-08-03T00:19:30Z

@aduth
code fixed & eslint added to repo

@iandunn
i18n data loading added

aduth · 2018-08-03T12:27:03Z

.eslintrc.js

@@ -0,0 +1,159 @@
+module.exports = {


This is prone to falling out of sync. Is okay for now, but we should try to push WordPress/gutenberg#7965 and update to use the single configuration.

yes waiting for eslint config to be published as a package will simplify things

aduth · 2018-08-03T12:28:30Z

.eslintignore

@@ -0,0 +1,8 @@
+build


Many of these don't exist in this repository:

build

build-module

coverage

packages/block-serialization-spec-parser

vendor

aduth · 2018-08-03T12:30:32Z

.gitignore

@@ -0,0 +1,9 @@
+# Directories/files that may be generated by this project


Many of these don't exist in this repository:

phpcs.xml

docker-compose.override.yml

aduth · 2018-08-03T12:32:18Z

01-basic/block.js

@@ -7,15 +7,17 @@
 * because all of these styles will appear in `post_content`.
 */
 ( function( blocks, i18n, element ) {
-	var el = element.createElement;
-	var __ = i18n.__;
+	const el = element.createElement;


These non-ESNext files should not include ES2015 (const)

oops that's eslint auto fix at work

oops that's eslint auto fix at work

We should probably configure it to ignore these directories if it's going to be prone to regressions by future maintainers running the script.

aduth · 2018-08-03T12:32:25Z

02-stylesheets/block.js

@@ -8,8 +8,8 @@
 * appropriate element with this class.
 */
 ( function( blocks, i18n, element ) {
-	var el = element.createElement;
-	var __ = i18n.__;
+	const el = element.createElement;


These non-ESNext files should not include ES2015 (const)

aduth · 2018-08-03T12:34:51Z

01-basic/index.php

 		'gutenberg-examples-01',
 		plugins_url( 'block.js', __FILE__ ),
 		array( 'wp-blocks', 'wp-i18n', 'wp-element' ),
 		filemtime( plugin_dir_path( __FILE__ ) . 'block.js' )
 	);
-}
+
+	if ( ! function_exists( 'register_block_type' ) ) {


Minor: This could be bumped up to even before we try to wp_register_script

aduth · 2018-08-03T12:38:08Z

04-controls/block.js

@@ -22,12 +23,15 @@
 				source: 'children',
 				selector: 'p',
 			},
+			alignment: {


Should this be implemented using the align support?

https://wordpress.org/gutenberg/handbook/block-api/#supports-optional

Support adds block alignment controls here we need the text alignment for rich text.

aduth · 2018-08-03T12:38:42Z

04-controls/block.js

-			);
+			return el( RichText.Content, {
+				tagName: 'p',
+				className: 'gutenberg-examples-align-' + props.attributes.alignment,


alignment could be undefined? In which case this would assign className of 'gutenberg-examples-align-undefined'

ajitbohra · 2018-08-03T20:44:34Z

@aduth

.gitignore , .eslintignore cleaned
es5 examples const replace with var
Gutenberg bailout moved to top
undefined alignment fixed

aduth · 2018-08-06T13:04:21Z

04-controls-esnext/block.js

-				{ props.attributes.content }
-			</p>
+			<RichText.Content
+				classes={ `gutenberg-examples-align-${ props.attributes.alignment }` }


classes doesn't exist. This should be className. Also this is prone to issue I mentioned earlier with alignment being undefined producing a class name of 'gutenberg-examples-align-undefined'. The previous condition protected against this.

ah my bad fixed the className prop, alignment undefined check was moved to click handler in previous commit

https://github.com/WordPress/gutenberg-examples/pull/23/files#diff-25a14e7ee681fc6559b787483e120fc2R43

So it's not quite the same in that previously we wouldn't assign a class if there wasn't an explicit alignment, and now we'd apply gutenberg-examples-align-none, which I guess could be useful for demonstration purposes of the default.

aduth · 2018-08-13T13:37:38Z

04-controls-esnext/block.js

-		const onChangeAlignment = newAlignment => {
-			props.setAttributes( { alignment: newAlignment } );
+		const onChangeAlignment = ( newAlignment ) => {
+			props.setAttributes( { alignment: newAlignment === undefined ? 'none' : newAlignment } );


Makes me wonder: Shouldn't explicitly setting an attribute to undefined cause its default value to be used automatically, rather than needing to duplicate it here? I'm not sure if this works as-is, and if it doesn't, would be worth creating an issue in the Gutenberg repository.

will test that case

aduth · 2018-08-13T13:38:40Z

04-controls-esnext/block.js

-				{ props.attributes.content }
-			</p>
+			<RichText.Content
+				classes={ `gutenberg-examples-align-${ props.attributes.alignment }` }


So it's not quite the same in that previously we wouldn't assign a class if there wasn't an explicit alignment, and now we'd apply gutenberg-examples-align-none, which I guess could be useful for demonstration purposes of the default.

aduth

I see some local changes to package-lock.json when running npm install, which are likely related to mismatches in npm version (I'm currently running 6.2.0). I'm not going to stress it too much.

Thanks for taking on these changes, and for the patience in bearing through the review process.

aduth · 2018-08-13T13:43:56Z

package.json

@@ -0,0 +1,25 @@
+{
+	"name": "gutenberg-examples",
+	"version": "3.4.0",


Is this versioning meant to follow Gutenberg? Out of date now 😄

iry47 · 2019-04-24T12:31:12Z

Hi guys, I'm getting the Unexpected Token "<" error and I saw that this thread had it solved. I've included all the updated files but am still getting the error. What was the exact reason & solution for this error ?

Update for gutenberg 2.9 compatibility

68f28d7

Soean reviewed May 31, 2018

View reviewed changes

Add text domain & setLocale

a52df7c

ajitbohra changed the title ~~Update for gutenberg 2.9 compatibility~~ Update for latest gutenberg compatibility Jun 25, 2018

ajitbohra requested a review from youknowriad June 26, 2018 13:47

skorasaurus mentioned this pull request Jul 6, 2018

2 Editable examples not working (step 3 and 4) #22

Closed

aduth reviewed Jul 9, 2018

View reviewed changes

Fix coding standards

5c0d85d

aduth reviewed Jul 12, 2018

View reviewed changes

iandunn reviewed Jul 13, 2018

View reviewed changes

ajitbohra added 3 commits August 1, 2018 14:14

Config eslint & fix code

feb53a7

Merge branch 'master' of github.com:ajitbohra/gutenberg-examples

f4b1aba

Load locale data & refactor assets loading

151bd61

This was referenced Aug 3, 2018

React error #31

Closed

Undefined variable #30

Closed

Uncaught SyntaxError: Unexpected token < #29

Closed

aduth reviewed Aug 3, 2018

View reviewed changes

ajitbohra added 4 commits August 3, 2018 20:40

Clean .gitignore & .eslintignore

2518aae

Use var for es5 examples instead of const

112440d

Move gutenberg check to top

87b8161

Handle undefined alignment

455d715

swissspidy mentioned this pull request Aug 5, 2018

Include build tool configs for i18n #27

Closed

aduth requested changes Aug 6, 2018

View reviewed changes

Merge branch 'master' into master

91c1699

ajitbohra added 2 commits August 13, 2018 09:12

Fix className on save

0f82a2d

Merge branch 'master' of github.com:ajitbohra/gutenberg-examples

eb2d489

aduth reviewed Aug 13, 2018

View reviewed changes

aduth approved these changes Aug 13, 2018

View reviewed changes

aduth merged commit 83d6fcf into WordPress:master Aug 13, 2018

		@@ -0,0 +1,9 @@
		# Directories/files that may be generated by this project

Update for latest gutenberg compatibility #23

Update for latest gutenberg compatibility #23

Conversation

ajitbohra commented May 31, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ajitbohra commented Jun 4, 2018

Soean commented Jun 4, 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

ajitbohra commented Jul 9, 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

iandunn Jul 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ajitbohra commented Jul 13, 2018

ajitbohra commented Aug 3, 2018

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

ajitbohra commented Aug 3, 2018 • edited Loading

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 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iry47 commented Apr 24, 2019

ajitbohra commented May 31, 2018 •

edited

Loading

iandunn Jul 13, 2018 •

edited

Loading

ajitbohra commented Aug 3, 2018 •

edited

Loading