FormRow: enable partial rendering #4412

Closed
wants to merge 2 commits into
from

Projects

None yet

4 participants

@Slamdunk
Contributor
Slamdunk commented May 3, 2013

Issue #4405

This is what I think @davidwindell intended to do, a way to use custom partial in the form row helper.

If you're thinking about why using a partial within a helper instead of directly call the partial helper in a view, I think that this is a way to easy override a lot of views leaving the $this->formRow($element); code in them.

The view has every helper it heeds to render whatever you want, so we can call the return $this->view->[...] immediately, instead of after some logic.

@davidwindell
Contributor

The view has every helper it heeds to render whatever you want, so we can call the return $this->view->[...] immediately, instead of after some logic.

Please retain passing of the label to the partial, it's helpful to avoid having to manually handle translation, etc. Also, please bring back the option to pass aditional custom vars to the partial, it's useful if you want a specific row to be slightly different to all others using the same partial (see $postfix in my example below).

Here's the partial I currently use showing use cases for the label and custom vars;

<div class="row<?php echo $errors ? " error" : "" ?>">
    <div class="three mobile-one columns">
        <?php if (isset($label)) : ?>
          <?php echo $label ?>
        <?php endif; ?>
    </div>
    <div class="nine mobile-three columns">
        <?php echo $element ?>
        <?php if ($errors): ?>
            <small><?php echo $errors ?></small>
        <?php endif; ?>
    </div>
</div>
@weierophinney
Member

@Slamdunk have you had a chance to try and incorporate the feedback from @davidwindell ? Do you need assistance? I'd like to get this in for RC2 if we can, but I'm also aiming for RC2 later today...

@Slamdunk
Contributor
Slamdunk commented May 6, 2013

I've passed the translated label to the partial.

Regarding the custom variables, I'm not very happy to take the partial template approach like you did.

Once you set the partial template, it will be always taken by next calls till someone will reset it to null, and this is okay because it's likely the behaviour we want: to set a custom row template for all.

But variables are different: if we set them in one row, we have to reset them in the next row with your old code, and i'm not okay with that.

A different solution may be to modify render function, or something, but this is an RC and no public interface change will be accepted.

@weierophinney to me there are only 3 possibilities:

  1. delete all this code and move forward this PR to another release
  2. accept this PR with limited but tested new functionality
  3. someone other than me decide what to do with custom variables :|
@Slamdunk
Contributor
Slamdunk commented May 6, 2013

The build is passing as of ZF2 PR https://travis-ci.org/zendframework/zf2/builds/6927579

The fail refers to the mine https://travis-ci.org/Slamdunk/zf2/builds/6927574 (I don't know why one passes and the other doesn't)

@weierophinney
Member

to me there are only 3 possibilities:

  1. delete all this code and move forward this PR to another release
  2. accept this PR with limited but tested new functionality
  3. someone other than me decide what to do with custom variables :|

I think 2 makes sense -- it brings the primary intent into place, and, as you noted, is tested. It's unlikely the API will need to change in the future in terms of basic usage. @davidwindell can then work on (3) later. :)

@weierophinney weierophinney was assigned May 6, 2013
@weierophinney weierophinney added a commit that closed this pull request May 6, 2013
@weierophinney weierophinney Merge branch 'hotfix/4412'
Close #4412
Fixes #4405
0941d08
@weierophinney weierophinney added a commit that referenced this pull request May 6, 2013
@weierophinney weierophinney Merge branch 'hotfix/4412' into develop
Forward port #4412
8342fcc
@glen-84
Contributor
glen-84 commented May 6, 2013

Guys, what about having a default view partial, so that this can be set in a bootstrap (for example), and then specifying a partial for a specific form row would use it for that row only, and then reset it to the default?

@davidwindell
Contributor

Thanks @Slamdunk for finishing this one off, for those looking in the future, an updated example of the partial I show above that produces a Foundation CSS style formRow;

<?php $errors = $this->formelementerrors($element); ?>
<div class="row<?php echo $errors ? " error" : "" ?>">
    <div class="three mobile-one columns">
        <?php if ($element->getLabel()) : ?>
          <?php echo $this->formlabel($element) ?>
        <?php endif; ?>
    </div>
    <div class="nine mobile-three columns">
        <?php echo $this->formelement($element); ?>
        <?php if ($errors): ?>
            <small><?php echo $errors ?></small>
        <?php endif; ?>
    </div>
</div>

in your module.php;

'formrow' => function ($sm) {
    $formRow = new FormRowHelper();
    $formRow->setPartial('partials/formrow.phtml');
    return $formRow;
}
@glen-84
Contributor
glen-84 commented May 7, 2013

@weierophinney, @Slamdunk

And another thing: Element errors are needlessly rendered when using a partial.

Suggestion:

Move:

$elementErrors   = $elementErrorsHelper->render($element);

... below the partial rendering, and change:

if (!empty($elementErrors)

... to:

if (count($element->getMessages()) > 0
@weierophinney
Member

@glen-84 Can you create a PR for that, please?

@glen-84
Contributor
glen-84 commented May 7, 2013

@weierophinney

#4441 ... I also checked $this->renderErrors before rendering.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment