Skip to content
This repository has been archived by the owner on Nov 6, 2022. It is now read-only.

Repeater row count and row index function. #429

Merged
merged 14 commits into from
Sep 17, 2019

Conversation

lukecarbis
Copy link
Member

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:

if ( block_rows( 'repeater' ) && block_row_count( 'repeater' ) > 3 ):

    while ( block_rows( 'repeater' ) ) :
        block_row( 'repeater' );

        // Note: The index is zero-based, so we should add 1 to get a non-programmer count.
        echo '<li>This is row number ' . block_row_index() + 1 . '</li>';
    endwhile;

    echo '</ul>';
endif;

Closes #403.

php/helpers.php Outdated
function block_row_count( $name ) {
global $block_lab_attributes;

if ( ! isset( $block_lab_attributes[ $name ] ) ) {
Copy link
Collaborator

@kienstra kienstra Sep 13, 2019

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;
}

Copy link
Member Author

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:

  1. Check if a row even exists
  2. 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.

Copy link
Collaborator

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in 6300f33.

@@ -183,6 +183,18 @@ public function cast_value( $value ) {
break;
}

if ( 'repeater' === $this->control ) {
Copy link
Collaborator

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in a1bb136.

@lukecarbis
Copy link
Member Author

@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 Field class, and move it into the Repeater control. If you don't think it makes much sense, though, I'd be happy to revert this commit.

*
* @var string
*/
public $type = 'object';
public $type = 'array';
Copy link
Collaborator

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.

Copy link
Member Author

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.

@kienstra
Copy link
Collaborator

Hi @lukecarbis,
Nice work here. I have to go to bed, and I'll take a look at it again tomorrow.

Copy link
Collaborator

@kienstra kienstra left a 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.

$rows = $attributes[ $field->name ]['rows'];
}

if ( isset( $attributes[ $field->name ]['rows'] ) ) {
Copy link
Collaborator

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in 39fade9.

Copy link
Collaborator

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'] ) ) {
Copy link
Collaborator

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 '' => ''

Copy link
Member Author

@lukecarbis lukecarbis Sep 16, 2019

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.

Copy link
Collaborator

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.

@kienstra
Copy link
Collaborator

Like we talked about, this should be good to merge whenever you'd like 😄

@lukecarbis lukecarbis merged commit 7893515 into develop Sep 17, 2019
@lukecarbis lukecarbis deleted the feature/repeater-row-count branch September 17, 2019 03:01
@lukecarbis lukecarbis mentioned this pull request Oct 28, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Repeater row count
2 participants