-
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] Add --variant
flag to allow creation of different block type variants
#41289
[Create-block] Add --variant
flag to allow creation of different block type variants
#41289
Conversation
…ow what packages are being installed.
…anaging both block type options.
…package to manage output based on the --is-dynamic flag.
Based on a conversation with @gziolo during contributor day at WCEU, this PR now uses the {{isDynamic}} template placeholder to conditionally out parts of the templates. It also now checks to see if a file is empty before writing it to the filesystem. This allows template authors to handle the use-case of files that should only be included depending on the type of block by allowing them to wrap the content in the conditional placeholders:
|
--is-dynamic
flag to allow creation dynamic bocks--is-dynamic
flag to allow creation of dynamic bocks
…and it's value is settings that will override the defaults.
…he attribute settings.
@gziolo I've addressed your notes from the code review. I've also made a change to how we use |
static: { | ||
slug: 'example-static-es5', | ||
title: 'Example Static Block (ES5)', | ||
}, |
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.
The values for the static
variant could be set directly in the defaultValues
object.
The same applies to the templates related to the standard
template.
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 was pondering one more optimization from the perspective of the developer writing a scaffolding template. What if we assume that everything that is included in defaultValues
is the default variant, so we never really need to pick it explicitly? If there are any variants, then we use the variants
option, but we only provide non-default variants:
variants: {
dynamic: {
slug: 'example-dynamic-block',
title: 'Example Dynamic Block',
},
anotherVariant: {},
},
That would generate 3 vars to use with the template:
isDefaultVariant
isDynamicVariant
isAnotherVariant
If no variants were included, we wouldn't have any variables injected into templates.
This would also remove some checks like whether there is more than one variant provided because we would always have the default one and one or more in the variants
field.
The only drawback is that we wouldn't have a way to pass the alternate name for the default variant so users won't see the radio option like:
- static
- dynamic
- anotherVariant
Instead, it would look like
- default
- dynamic
- anotherVariant
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 like the idea, perhaps we can iterate on 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.
I moved the default values from static
to defaultValues
in #43481. I didn't refactor further. It's still valuable to have a way to name the default variant. In the future, we could also figure out how to pass some sort of description for the variant. At the moment it's static
vs dynamic
which might not be clear for someone starting with WP blocks.
Excellent follow-up @ryanwelcher. Brilliant idea to use an object with overrides for variants 💯 We now only need to settle on the final shape of variants, and we should be good to go. I still need to re-test the plugin after applying all the changes. |
Code-wise everything looks good. There is one issue that I flagged in https://github.com/WordPress/gutenberg/pull/41289/files#r949815229 that I consider a blocker so it needs to get resolved. |
I believe I have addressed this. Please retest when you can. |
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.
It looks good. Let's get this one in after all those iterations 🎉
I'm running some extensive testing and open follow-up PR with enhancements if necessary.
] ).filter( filterOptionsProvided ); | ||
|
||
// Get the variant prompt first. This will help get the default values | ||
const variantPrompt = |
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.
When I provide the variant from the CLI, it asks again for the variant. We can address it seperately.
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.
Fixed in #43481.
What?
This PR introduces a new CLI flag to the
create-block
tool to indicate if the generated block should be dynamic or static. The default state is to create a static block and by passing--variant dynamic
a dynamic block would be created.Why?
There is currently no way to scaffold a dynamic block using
create-block
without using a custom template. The intent here is to allow any template, including the built-in ones, to support creating either type of block.How?
I have added the new flag,
--variant
, and am currently using a combination of Mustache template conditionals and custom-named templates to load the correct files.Testing Instructions
node index.js my-test --variant dynamic
from the root of the create-block packagetemplate.php
file added to the src directory and that nosave.js
file was added or is referenced inindex.js
node index.js my-test
from the root of the create-block package and confirm thatsave.js
is in-place and references correctly inindex.js