-
Notifications
You must be signed in to change notification settings - Fork 317
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
Update for latest gutenberg compatibility #23
Conversation
01-basic/block.js
Outdated
@@ -17,25 +17,25 @@ | |||
}; | |||
|
|||
blocks.registerBlockType( 'gutenberg-examples/example-01-basic', { | |||
title: __( 'Example: Basic', 'gutenberg-examples' ), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are you removing the textdomain?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not all examples have text domain and currently it throws console error as we are not loading translations.
@Soean |
Perfect, these examples should be best practices, so we should add i18n. Thanks :-) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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/
01-basic/block.js
Outdated
@@ -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 ) { | |||
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excess whitespace at end of line.
01-basic/block.js
Outdated
*/ | ||
( function( blocks, i18n, element ) { | ||
*/ | ||
(function (blocks, i18n, element) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't be space after function
. Should be space before function
.
01-basic/block.js
Outdated
return el( | ||
'p', | ||
{ style: blockStyle }, | ||
'Hello World, step 1 (from the frontend).' | ||
); | ||
}, | ||
} ); | ||
} )( | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This spacing shouldn't be collapsed.
@aduth thanks for the review coding standards fixed. Is it good to merge? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We really need ESLint on this repository 😬
01-basic/block.js
Outdated
} ); | ||
} )( | ||
} | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still whitespace issue here; space after }
.
02-stylesheets/block.js
Outdated
} ); | ||
} )( | ||
} | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still whitespace issue here; space after }
.
03-editable/block.js
Outdated
var children = blocks.source.children; | ||
var RichText = editor.RichText; | ||
|
||
wp.i18n.setLocaleData({ '': {} }, 'gutenberg-examples' ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still whitespace issue here; space after setLocaleData(
.
03-editable/block.js
Outdated
@@ -19,36 +20,38 @@ | |||
content: { | |||
type: 'array', | |||
source: 'children', | |||
selector: 'p', | |||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Trailing commas are desirable (enforced in Gutenberg by ESLint)
03-editable/block.js
Outdated
tagName: 'p', value: props.attributes.content | ||
}); | ||
} | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still whitespace issue here; space after }
.
05-recipe-card-esnext/block.js
Outdated
edit: props => { | ||
const focusedEditable = props.focus ? props.focus.editable || 'title' : null; | ||
const attributes = props.attributes; | ||
edit: (props) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still whitespace issue here; space between parentheses.
01-basic-esnext/block.js
Outdated
@@ -7,8 +7,10 @@ const blockStyle = { | |||
padding: '20px', | |||
}; | |||
|
|||
setLocaleData( { '': {} }, 'gutenberg-examples' ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this need to include the localized strings and current locale via gutenberg_get_jed_locale_data()
?
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' );
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
@aduth yes it would be good to add eslint will update to include one. seems like my eslint/prettier is messing with those spaces. |
@@ -0,0 +1,159 @@ | |||
module.exports = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes waiting for eslint config to be published as a package will simplify things
.eslintignore
Outdated
@@ -0,0 +1,8 @@ | |||
build |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Many of these don't exist in this repository:
build
build-module
coverage
packages/block-serialization-spec-parser
vendor
@@ -0,0 +1,9 @@ | |||
# Directories/files that may be generated by this project |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Many of these don't exist in this repository:
phpcs.xml
docker-compose.override.yml
01-basic/block.js
Outdated
@@ -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; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These non-ESNext files should not include ES2015 (const
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oops that's eslint auto fix at work
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
02-stylesheets/block.js
Outdated
@@ -8,8 +8,8 @@ | |||
* appropriate element with this class. | |||
*/ | |||
( function( blocks, i18n, element ) { | |||
var el = element.createElement; | |||
var __ = i18n.__; | |||
const el = element.createElement; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These non-ESNext files should not include ES2015 (const
)
01-basic/index.php
Outdated
'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' ) ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor: This could be bumped up to even before we try to wp_register_script
@@ -22,12 +23,15 @@ | |||
source: 'children', | |||
selector: 'p', | |||
}, | |||
alignment: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be implemented using the align
support?
https://wordpress.org/gutenberg/handbook/block-api/#supports-optional
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Support adds block alignment
controls here we need the text alignment
for rich text.
); | ||
return el( RichText.Content, { | ||
tagName: 'p', | ||
className: 'gutenberg-examples-align-' + props.attributes.alignment, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
alignment
could be undefined? In which case this would assign className
of 'gutenberg-examples-align-undefined'
|
04-controls-esnext/block.js
Outdated
{ props.attributes.content } | ||
</p> | ||
<RichText.Content | ||
classes={ `gutenberg-examples-align-${ props.attributes.alignment }` } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah my bad fixed the className prop, alignment undefined check was moved to click handler in previous commit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
const onChangeAlignment = newAlignment => { | ||
props.setAttributes( { alignment: newAlignment } ); | ||
const onChangeAlignment = ( newAlignment ) => { | ||
props.setAttributes( { alignment: newAlignment === undefined ? 'none' : newAlignment } ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will test that case
04-controls-esnext/block.js
Outdated
{ props.attributes.content } | ||
</p> | ||
<RichText.Content | ||
classes={ `gutenberg-examples-align-${ props.attributes.alignment }` } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -0,0 +1,25 @@ | |||
{ | |||
"name": "gutenberg-examples", | |||
"version": "3.4.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this versioning meant to follow Gutenberg? Out of date now 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no idea :p
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 ? |
Fixes console errors, warnings & outdated code.
Also, addresses following issues
Fixes #31
Fixes #30
Fixes #29
Fixes #25
Fixes #22
Fixes #21