-
Notifications
You must be signed in to change notification settings - Fork 63
Repeater row count and row index function. #429
Conversation
php/helpers.php
Outdated
function block_row_count( $name ) { | ||
global $block_lab_attributes; | ||
|
||
if ( ! isset( $block_lab_attributes[ $name ] ) ) { |
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 function looks good overall.
Is this conditional necessary?
It seems that if it's not set, the block below will return false:
if ( ! isset( $block_lab_attributes[ $name ]['rows'] ) ) {
return false;
}
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.
That's a really good point.
The thinking here is:
- Check if a row even exists
- Check if it's a repeater (has rows)
Given this, do you still think it's worth only doing the single (second) condition? I'm more than happy to make that change. Just wanted to explain my approach.
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.
Sure, it's kind of a nitpick 😄
My guess is that there's not much performance difference between the existing approach, and an approach with only one isset()
check.
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 6300f33.
php/blocks/class-field.php
Outdated
@@ -183,6 +183,18 @@ public function cast_value( $value ) { | |||
break; | |||
} | |||
|
|||
if ( 'repeater' === $this->control ) { |
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.
Could the logic in this class be in the Repeater class, in a validate()
method?
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.
Done in a1bb136.
@kienstra Thanks for your review. Ready for you to take another look. Wanted to make note of 8d0b4f6. I was trying to remove repeater specific functionality from the |
* | ||
* @var string | ||
*/ | ||
public $type = 'object'; | ||
public $type = 'array'; |
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.
Good thing to bring up. In PHP, this would be an array()
, and in JS it'd be an object. I don't see many examples of array or object attributes in Core blocks.
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 – makes much more sense for $type
to represent the Javascript type, rather than the PHP type. Addressed in 2b61b64.
Hi @lukecarbis, |
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
Some Minor Points
Hi @lukecarbis,
This looks good. The new functions should be really useful.
There are just a few minor points. I'll be in and out today, so feel free to merge when you're happy.
php/blocks/class-loader.php
Outdated
$rows = $attributes[ $field->name ]['rows']; | ||
} | ||
|
||
if ( isset( $attributes[ $field->name ]['rows'] ) ) { |
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.
Maybe this conditional could be moved above to line 309
, instead of creating a new if
block:
if ( isset( $field->settings['sub_fields'] ) && isset( $attributes[ $field->name ]['rows'] ) ) {
Right now, it's possible (though unlikely) for $sub_field_settings
to be unset at line 318.
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.
Good catch.
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 39fade9.
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.
Thanks 😄
* @return mixed $value The value to be made available or echoed on the front-end template. | ||
*/ | ||
public function validate( $value, $echo ) { | ||
if ( isset( $value['rows'] ) ) { |
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.
How about something like:
array_filter( $value['rows'] );
It looks like that removes empty array values, like '' => ''
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 would be nice, except that array_filter
filters empty values (not empty keys) – so if there were a repeater with an empty value, that item in the array would be removed.
We could supply a custom callback to array_filter which targets empty keys, or alternatively use something like this:
array_diff_key( $value['rows'][ $key ], array_flip( array( '', 0 ) ) );
but ultimately, I think just unsetting the two is a better, simpler, solution.
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, it removes empty values. That's not good.
Like we talked about, this should be good to merge whenever you'd like 😄 |
This adds some helper functions for retrieving the total count of repeater rows, and the index of an individual repeater row.
The helper functions are:
block_row_count( $name )
This takes a repeater field name as the argument, and returns the row count for that repeater. (It can also return false, if you pass it an invalid repeater name.)
block_row_index()
This should be used inside a repeater loop (after the
block_row()
call). This returns the index of the current row.Here's a code example:
Closes #403.