Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

individual attributes for each option (multi checkbox) #48

Open
wants to merge 3 commits into
base: 2.x
Choose a base branch
from

Conversation

rodsouto
Copy link
Contributor

@rodsouto rodsouto commented Mar 8, 2016

Hi guys, today I needed to set some data-attributes for a particular input in a multi checkbox, are you interested in this feature? :)

@harikt
Copy link
Member

harikt commented Mar 9, 2016

@rodsouto isn't it the same one you send #46 ?

@harikt
Copy link
Member

harikt commented Mar 9, 2016

Sorry I understood it wrongly :) . Thanks for the PR.

@@ -80,10 +107,12 @@ protected function multiple()
$this->attribs['value'] = $value;
$this->attribs['label'] = $label;

$optionAttribs = isset($this->optionsAttribs[$value]) ? $this->optionsAttribs[$value] : [];
Copy link
Member

Choose a reason for hiding this comment

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

Travis broken due to 5.3 . Need to change short array to array.

@rodsouto
Copy link
Contributor Author

rodsouto commented Mar 9, 2016

Thanks @harikt 👍

@@ -80,10 +107,12 @@ protected function multiple()
$this->attribs['value'] = $value;
$this->attribs['label'] = $label;

$optionAttribs = isset($this->options_attribs[$value]) ? $this->options_attribs[$value] : array();
Copy link
Member

Choose a reason for hiding this comment

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

Probably here also :) .

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@harikt
Copy link
Member

harikt commented Jul 3, 2016

Hey @jakejohns ,

Do you think we are good to merge this ?

@jakejohns
Copy link
Member

I like the concept. Here's some concerns off the top of my head:

Update Docs

README-FORMS.md needs updating. In fact, I don't think the "multiple" function is even documented there, unless i'm missing it.

Should this apply to other helpers?

I wonder if similar function should be applied to Radio and Select options. I think Radio might be as easy as moving the prep of this to AbstractChecked and doing a similar thing to the multiple method. I think Select might be more difficult, and could wait if so

Spec Structure

I actually think the way it's being done here is probably the right option, but I just thought I'd throw this out there to explicitly rule it out for a valid reason. I haven't messed with the Checked helpers too much, so I very well may be missing something here.

I sort of wish the attrs weren't separate from the other content, requiring the keys to match in different arrays. But It's probably not a good idea to mess with the spec structure too much, right? eg. this is worse, or wouldn't work, right? Maybe not?:

<?php

// calling
$actual = $checkbox(array(
    'name' => 'foo',
    'value' => 'yes',
    'options' => array(
        'yes' => array(
            'label' => 'Yes',
            'attr' => array('class' => 'test-class')
        ),
        'no' => array(
            'label' => 'No',
            'attr' => array('data-no' => 1)
        ),
    )
));


// Checkbox.php ... totally untested
protected function multiple()
{
    $html = '';
    $checkbox = clone($this);

    $this->attribs['name'] .= '[]';

    foreach ($this->options as $value => $label) {

        $attr = array();

        if (is_array($label)) {
            $attr = $label['attr'];
            $label = $label['label'];
        }

        $this->attribs['value'] = $value;
        $this->attribs['label'] = $label;

        $html .= $checkbox(array(
            'name'    => $this->attribs['name'],
            'value'   => $this->value,
            'attribs' => $this->attribs + $attr
        ));
    }
    return $html;
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants