adamwathan / form

Super basic form HTML builder, only really exists so I can pull it in for some other more useful projects.
MIT License
232 stars 117 forks source link

New method for setting multiple on selects. #47

Closed jesseleite closed 9 years ago

jesseleite commented 9 years ago

Chaining multiple() method: $builder->select('people')->multiple();

Generates: <select name="people[]" multiple="multiple"></select>;

Great for tags, etc. Example

coveralls commented 9 years ago

Coverage Status

Coverage increased (+0.06%) to 94.67% when pulling df7236291e4a0177a49c6320b7c7c4abafed64f7 on JerseyMilker:select-multiple into cff7aff5a7f7e213091053d2134d3db0d605f2fc on adamwathan:master.

adamwathan commented 9 years ago

I dunno if I like automatically appending the brackets, it seems easy enough to do $builder->select('people[]')->multiple() and I'd be worried it would be a pain if there was ever a situation where someone didn't want to have the brackets added, or wanted to control it manually.

Right now the package already automatically treats missing method calls as attribute setters, so multiple('multiple') does what you want to do here, which makes me think it might be a good idea to always pass the attribute name as the value for the attribute if a missing method is ever called with no parameters, you know?

That would give you this same API:

$builder->select('people[]')->multiple()

...without ever having to declare the multiple method on the select class.

Basically just means doing this in the Element class instead of what we have currently:

    public function __call($method, $params)
    {
        $params = count($params) ? $params : array($method);
        $params = array_merge(array($method), $params);
        call_user_func_array(array($this, 'attribute'), $params);
        return $this;
    }

I think I like that solution better personally because it can be applied to things like "required" and stuff too.

If you like that approach feel free to update the PR so you get the credit :)

jesseleite commented 9 years ago

I agree with making the magic method param optional, so that an empty required() method is same as required('required'), etc.

Reason I don't like manually adding [] in constructor method like this:

$builder->select('people[]')->multiple()

Is because I'm thinking in context of BootForms. If I do as you suggest:

{!! BootForm::select('Names', 'names[]')->options(['Jesse Leite', 'Adam Wathan'])->multiple('multiple') !!}

Then 'names[]' with square brackets get rendered into more than just the name attribute:

<div class="form-group">
    <label class="control-label" for="names[]">Names</label>
    <select name="names[]" id="names[]" class="form-control" multiple="multiple">
        <option value="0">Jesse Leite</option>
        <option value="1">Adam Wathan</option>
    </select>
</div>

See id and for attributes. I assume this would affect bootstrap help blocks for validation handling as well. Allowing the multiple() method to handle the [], means that the other attributes aren't affected when using extensions like BootForm. And more food for thought: When would you ever use [] without multiple="multiple", or vice versa?

That said, if you want the option to manually $builder->select('people[]'), the Select class' multiple() method could regex for [] and only add to name if it doesn't already exist in name. This could be a best of both worlds solution? Your thoughts?

coveralls commented 9 years ago

Coverage Status

Coverage increased (+0.13%) to 94.74% when pulling f1e662454a8cf14daf49d69676721d226f982858 on JerseyMilker:select-multiple into cff7aff5a7f7e213091053d2134d3db0d605f2fc on adamwathan:master.

jesseleite commented 9 years ago

I committed my suggestion anyway.

This satisfies your requests:

This satisfies my requests:

Your thoughts? Let me know when you Form them ;)

adamwathan commented 9 years ago

Looks good after addressing my comments above :+1:

jesseleite commented 9 years ago

Good call, committed change regarding existing name attribute.

Also, I already have an assertion which tests for existing [] in multi-select name:

public function testSelectCanBeMultiple()
{
    $select = new Select('people');  // Passing name without [].
    $expected = '<select name="people[]" multiple="multiple"></select>';
    $result = $select->multiple()->render();

    $this->assertEquals($expected, $result);

    $select = new Select('people[]');  // Passing name with [].
    $expected = '<select name="people[]" multiple="multiple"></select>';
    $result = $select->multiple()->render();

    $this->assertEquals($expected, $result);
}
coveralls commented 9 years ago

Coverage Status

Coverage increased (+0.11%) to 94.72% when pulling 8e5968da46cd8a2f01ca461a4069ee9408cd50af on JerseyMilker:select-multiple into cff7aff5a7f7e213091053d2134d3db0d605f2fc on adamwathan:master.

jesseleite commented 9 years ago

<3