-
Notifications
You must be signed in to change notification settings - Fork 64
PHP API for registering blocks with Block Lab #434
Conversation
Hi @lukecarbis, If it's alright, tomorrow is probably the earliest I could review 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.
Looks Good, Question About Adding Method To Loader
Hi @lukecarbis,
This looks good, it's a big win for the plugin.
What do you think about moving the logic from the add_filter()
calls into a new method in the Loader
class?
This and #435 depend on the Loader
class, but before, it wasn't really available in any other file.
#440 moves it into block_lab()->loader
, and makes most of the method protected.
Either way, hope you're doing great, and look forward to talking tomorrow.
php/helpers.php
Outdated
$block_config['keywords'] = array(); | ||
} | ||
|
||
if ( ! isset( $block_config['fields'] ) ) { |
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.
wp_parse_args() might make this a little easier
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.
Address in e547cc3.
php/helpers.php
Outdated
$block_config['fields'] = array(); | ||
} | ||
|
||
add_filter( |
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 logic in the callback looks good.
What do you think about adding a public function to class Loader
like:
block_lab()->loader->add_block( $block_config );
Then, it could store the registered blocks in a protected property of Loader
.
That could have most of the logic in this callback, but this particular add_filter()
call wouldn't be needed.
The apply_filters( 'block_lab_blocks' )
call in Loader
would still be useful, as would this block_lab_add_block()
function.
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.
Addressed in 5b98f71.
php/helpers.php
Outdated
$field_config['settings'] = array(); | ||
} | ||
|
||
add_filter( |
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.
Like the comment above, what do you think about turning this add_filter()
call into something like:
block_lab()->loader->add_field( $block_name, $field_config );
...or whatever method name.
It could then add fields to blocks added via the possible function above: block_lab()->loader->add_block( $block_config );
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, that wouldn't be able to add fields to existing blocks created with /wp-admin
> Block Lab > Add New.
Would that be a common use case, to add a block via via /wp-admin
> Block Lab > Add New, then later add a field to it with block_lab_add_field()
?
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 don't think it would be a very common use case, no.
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.
Addressed in 5b98f71.
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.
Nice, that looks good. Thanks for pushing it.
@kienstra Thanks for you're review! Great to have your eyes on it, great suggestions. I've made some changes to address your comments. Could you please take another look? |
Hi @lukecarbis, this looks really good. I think the logic is good to go 😄 Would you mind if I pushed some unit tests to this branch just for completeness? For example, I was thinking of suggesting we eventually change to |
Please feel free to push unit tests. |
Thanks, Luke! |
Add assertions that these methods work. Also, make a slight change to add_field().
The conflicts were trivial. For test-class-loader.php, I retained both edits. It class-abstract-template.php, I used the edits from the develop branch. They were only to the @param tag in a DocBlock.
Test a case where there is not 'type' passed, where it should add the type if the control exists.
…n Travis This might not work well in Travis, depending on how the version work together, like the phpunit version.
This tests using the second argument, with a long name.
This is similar to block_lab_add_block(), and uses Mockery.
@@ -9,6 +9,7 @@ | |||
}, | |||
"require-dev": { | |||
"brain/monkey": "^2", | |||
"mockery/mockery": "^1.2.4", |
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 allows mocking entire classes, like Loader
. It doesn't look like Brain Monkey can mock classes.
Using the PHP API example above, the block:
|
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.
Great work!
Hi @lukecarbis,
This looks really good.
The API worked as expected, and I think it's ready to merge when you're ready.
* @param array $field_config The config of the field to add. | ||
*/ | ||
public function add_field( $block_name, $field_config ) { | ||
if ( ! isset( $this->blocks[ "block-lab/{$block_name}" ] ) ) { | ||
return; | ||
} | ||
if ( ! isset( $field_config['name'] ) ) { | ||
return $blocks; | ||
return; |
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 it alright that I made this change? Maybe something else was intended.
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, nothing else intended. Good pickup.
I've implemented the following functions, which enable to ability to register blocks programatically:
block_lab_add_block( $block_name, $block_config )
and
block_lab_add_field( $block_name, $field_name, $field_config )
Both are well documented in their respective doc blocks, if you'd like to learn more about these functions and how to use them.
They need to run during the
block_lab_add_blocks
action hook.Here's some example code I have been testing with:
Closes #400.