-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
[create-block] Refinements to the create-block-interactive-template package. #52801
[create-block] Refinements to the create-block-interactive-template package. #52801
Conversation
Size Change: +499 B (0%) Total Size: 1.44 MB
ℹ️ View Unchanged
|
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.
LGTM!
I added a couple of questions below 🙂
* @see https://github.com/WordPress/gutenberg/blob/trunk/docs/reference-guides/block-api/block-metadata.md#render | ||
*/ | ||
|
||
$unique_id = uniqid( 'p-' ); | ||
?> | ||
|
||
<div | ||
<?php echo get_block_wrapper_attributes(); ?> | ||
<?php echo wp_kses_data( get_block_wrapper_attributes() ); ?> |
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.
Are you sure this is needed? I've never seen it before, and none of the Core blocks seems to do it.
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.
There is some differing opinions on this. WPCS is trigger because all echo
statements need to be escaped. That being said, I see that the default template in create-block is not using it. So I'll remove it for consistency.
customBlockJSON: { | ||
example: { | ||
attributes: { | ||
message: 'Example Interactive', | ||
}, | ||
}, | ||
}, |
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, out of curiosity, what is this example
thing teaching the developer? How to add attributes?
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.
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 does beg the question of the point of that inner attributes
property. I don't think it's actually being used...
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.
Looks like by just having example
defined, the preview appear. If we were actually using attributes
, we'd need to add those as well. Change incoming.
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.
Ohhhh, nice.
I completely overlooked that example
property until now 😄
I've moved the |
Flaky tests detected in 5629261. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/5645662304
|
What?
This PR introduces some refinements to the template files to have the
example
property defined in theblock.json
file instead of theindex.js
and to add some late escaping to the render.php file.Why?
It is better to contain as much as possible in the block.json file, especially if there are translations involved as those strings are automatically translated. The escaping items trigger various WPCS rules and make for a better developer experience.
Testing Instructions
cd
into thepackages/create-block-interactive-template create-block-interactive-template
npx @wordpress/create-block@latest my-first-interactive-block --template ./
my-first-interactive-block
and runnpx @wp-now/wp-now start
]