Allow the formHelper->label() to have default options #598

Closed
wants to merge 2 commits into
from

Projects

None yet

3 participants

@nabeelio
nabeelio commented Apr 5, 2012

Ok, re-doing this request to the proper branch.

This allows any labels to have their default options set using a new option called 'labelDefaults'
It seems to me that 'label' is the only one missing this, as all other inputs have it.

I looked at the unit-tests for the formHelper, and I wasn't quite sure how to implement the tests, I can amend this commit with any tests, just advise me on the proper way to lay them out.

@nabeelio nabeelio allow the label to pick up default properties from the inputDefaults.…
… merges in any options and they can be overridden locally
c4208a6
Member
ceeram commented Apr 5, 2012

@nshahzad you dont need to amend your commit, you can just push extra commits to same branch and they will automatically be added to the pull request.
I am still not sure if we would want such a thing, we will end up with labelDefaults, errorDefaults etc

You can use Form->create('Model', array('inputDefaults' => array('label' => $labelDefaults)));

Just my opinion, lets see what others say

nabeelio commented Apr 5, 2012

What I mean is that this: Form->create('Model', array('inputDefaults' => array('label' => $labelDefaults))); doesn't work. There's no code in the label() function that merges the local-scoped $options with any $inputDefaults['label'] - that's what my original commit did - it merged in inputDefaults['label']

Right now, there is an 'error' under inputDefaults() - but perhaps that can also be extracted out, it's nested quite a bit inside. But either way (labelDefaults or just label) makes no difference to me, it would just be nice to have this functionality. I have a lot of repeated code now in every form for the input() to set some options for label. Just trying to keep 'DRY'.

But yes, I agree, let's see what others say.

Member
ceeram commented Apr 5, 2012

the inputDefaults are exactly to avoid repeated code in input()

See: 8c631fa

nabeelio commented Apr 5, 2012

Yup, I know. But they don't get applied to any label, which is the problem.

Member
ceeram commented Apr 5, 2012

look at the test, they will, when you use input()

nabeelio commented Apr 5, 2012

Okay, I think I see the problem.

When you override the label text, the options are lost, not when the label is auto-generated.

echo $this->Form->input('body', array(
   'label' => 'Description:',
   'type' => 'textarea', 'class' => 'span6', 'placeholder' => 'Enter a complete description of your item'
)); 

Which branch is that test in? I'll write a test that fails, hopefully that shows what I mean.

Member
ceeram commented Apr 5, 2012

Yes default input options can and will be overriden when you define input options again. It is designed to do so.
Edit: the testcase was added in 2.1, but you can add a testcase to this pull request as well, but it is supposed to fail as its designed to override a default setting

Member
ceeram commented Apr 5, 2012

I suppose this is what you would want to pass:

        $this->Form->create('User', array(
            'inputDefaults' => array(
                'div' => false,
                'label' => array('class' => 'nice', 'for' => 'changed'),
            )
        ));
        $result = $this->Form->input('username', array('div' => true, 'label' => 'Description'));
        $expected = array(
            'div' => array('class' => 'input text'),
            'label' => array('for' => 'changed', 'class' => 'nice'), 'Description', '/label',
            'input' => array('type' => 'text', 'name' => 'data[User][username]', 'id' => 'UserUsername'),
            '/div'
        );
        $this->assertTags($result, $expected);
Member
ceeram commented Apr 5, 2012

@nshahzad 47558e8 here there a similar thing reverted for the same reason. It made it impossible to undo inputDefaults. They are now just overridden again instead of merged
More info: http://cakephp.lighthouseapp.com/projects/42648/tickets/2723

nabeelio commented Apr 5, 2012

Yes, that's what I'd want to pass, but it requires me to reset all of the options because there's no merge.
A merge would be much better than an override; to undo the input defaults, wouldn't you just set that option explicitly?

Setting a 'labelDefaults' makes it so inputDefaults aren't affected, I think that's a good idea. But in my opinion, I don't think that behavior is different from what input does right now, in being able to override the options, just applies to the label as well since it's not nested.

@markstory markstory closed this Feb 2, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment