Form Helpers Suggestions

[CodingJungle]

I have a few suggestions, One i've made before, but i thought i'd dust it off.

1. can we have the "id" of the forms, if not set, bet set to the helpers name, with "_js" appended to it? Form helpers are used ALOT by us developers, it would be nice not to have to fill in 6 parameters, that all have different different default values, just to set the ID just so we can use "togglesOn" or "togglesOff" (or variant there of). the option would still be there in case we want to override the "default" id of name with "_js" appended. this would make the code a lot easier to read, especially if we are just using it for the toggles options. 

2. if the field is disabled, have it use a different _desc lang string, have it use something like form_name_desc_disabled, that way we can inform the user why the field is disabled.

3. allow us to define a different description lang key other than "form_name_desc", like we can do with $obj->label property (if you do option 2, allow us to also define a different disabled form_name_desc_disabled lang key as well, instead of a property, can we get these moved into the options parameter?).

I don't think adding these options in, would have any real backwards compatibility potential as it all can fall back to what you already have if they aren't set or aren't available (like a _disabled description).

thanks.

 

[bfarber]

1) Honestly, I don't feel it's needed personally. Yes you might hate having to define several parameters just to set the ID (I know it can get annoying) but you do it once during development and it's done. HTML IDs are supposed to be unique, so suppose you have two forms on the page that both have a form element with the name "member_name" - if we took your suggestion both would end up with the element ID "member_name_js" if you didn't manually define the ID, which is invalid. We can't really automatically set IDs to anything really useful for this reason.

2) There is already a workaround way to accomplish this. Take a look at \IPS\core\modules\admin\settings\advanced::_manageDatastore() where we create the radio list of caching options.

3) The method I highlighted for #2 should also accommodate this need, I'd assume. We really don't want to add in a bunch of complexity for something fairly simple if it's not necessary. It's just more code we have to maintain that we don't really have a need for at the end of the day. When something becomes necessary (there's no other way to accomplish something, or it's a huge task that shouldn't be necessary) that's one thing, but I think the workaround I pointed out here should be sufficient for your needs.

[CodingJungle]

6 minutes ago, bfarber said:

1) Honestly, I don't feel it's needed personally. Yes you might hate having to define several parameters just to set the ID (I know it can get annoying) but you do it once during development and it's done. HTML IDs are supposed to be unique, so suppose you have two forms on the page that both have a form element with the name "member_name" - if we took your suggestion both would end up with the element ID "member_name_js" if you didn't manually define the ID, which is invalid. We can't really automatically set IDs to anything really useful for this reason.

yes for you guys it might be a "one and done" situation, that you might not write them as often as say us 3rd parties. I use it heavily in almost every app/plugin i make. it also makes the code look messy and hard to read at times.

You also already do this in the code if its apart of a form, but it wont pass the validation (afaik this doesn't get passed to the validation to check if its "toggled" or not) so you can't use that "id" for toggles if the field is "required". so it looks like its already "mostly" in place, just too far down in the "chain" to make a difference.

$this->htmlId ?: ( $form ? "{$form->id}_{$this->name}" : NULL )

also in your templates, you scenario can easily play out as well. 

id="elInput_{{if ! empty($htmlId)}}{$htmlId}{{else}}{$name}{{endif}}"

if its not apart of a form, it will have elInput_{$name}, so if there are two elements with the same name, it would have a "invalid" ID cause there would be two elements on the page having the same ID.

So i honestly think this could be taken care of, if what you already do for forms by giving them an ID if one doesn't exist (it would create a unique ID and it would be predictable for my toggles list too) if it can be made to work with validation if its toggled (hidden) and is required or whatever. 

 

35 minutes ago, bfarber said:

2) There is already a workaround way to accomplish this. Take a look at \IPS\core\modules\admin\settings\advanced::_manageDatastore() where we create the radio list of caching options.

3) The method I highlighted for #2 should also accommodate this need, I'd assume. We really don't want to add in a bunch of complexity for something fairly simple if it's not necessary. It's just more code we have to maintain that we don't really have a need for at the end of the day. When something becomes necessary (there's no other way to accomplish something, or it's a huge task that shouldn't be necessary) that's one thing, but I think the workaround I pointed out here should be sufficient for your needs.

\IPS\Member::loggedIn()->language()->words["datastore_method_{$k}_desc"] = \IPS\Member::loggedIn()->language()->addToStack('datastore_method_disableddesc', FALSE, array( 'sprintf' => array( $k ) ) );

Sure that might work, but looking at the code entirely doesn't seem less "complex" than what i'm suggesting.

abstract class _FormAbstract {

//ips stuff

public $desc = null;

//some more ips stuff

public function rowHtml( $form=NULL )
{
	//other method stuff
	if( $this->desc === null ){
		$desc = $this->name."_desc";
	}
	else{
		$desc = $this->desc;
	}
}

}

here all form helpers would inherit this ability innately. guess i was looking for more elegance out the code than more hacks. 

thanks for looking at it. 

[bfarber]

For the IDs - you copied a bit of code

$this->htmlId ?: ( $form ? "{$form->id}_{$this->name}" : NULL )

Does this not achieve what you are after? I don't have the classes open right now to reference, but if you have a form object it's setting an ID which you said would be predictable for your purposes already. Or is this happening elsewhere in the code and not giving you what you need?

[CodingJungle]

2 hours ago, bfarber said:

For the IDs - you copied a bit of code

$this->htmlId ?: ( $form ? "{$form->id}_{$this->name}" : NULL )

Does this not achieve what you are after? I don't have the classes open right now to reference, but if you have a form object it's setting an ID which you said would be predictable for your purposes already. Or is this happening elsewhere in the code and not giving you what you need?

it partially does.if the helper gets hidden thru a toggles action and its required (not meeting the min. for validation) it will throw an error about the helper being required (even tho it's hidden). 

[HeadStand]

On 10/4/2016 at 11:03 AM, CodingJungle said:

it partially does.if the helper gets hidden thru a toggles action and its required (not meeting the min. for validation) it will throw an error about the helper being required (even tho it's hidden). 

That drives me NUTS. For hidden fields I usually end up setting them as $appearRequired=true and then putting a custom validation function to make sure it's populated. Hacky, but it works.

[CodingJungle]

5 hours ago, HeadStand said:

That drives me NUTS. For hidden fields I usually end up setting them as $appearRequired=true and then putting a custom validation function to make sure it's populated. Hacky, but it works.

Yeah i've ended up doing this too at times, but the form is fully capable of handling this on its own too with the toggles information. I see "appareRequired" more as a way to bypass the generic validation of 

		if( $this->value === '' and $this->required )
		{
			throw new \InvalidArgumentException('form_required');
		}

so you could do a more robust validation, but still make the user aware visually that it is required :). 

[bfarber]

So I'm clear, you are saying you have a field hidden by toggles but required, and when you submit it throws an error since no value is provided, even though it's hidden?

Are you able to reproduce this issue anywhere else within the Suite? Form helpers are pretty central code so I'd assume the same issue must affect the release itself as well?

[Marcher Technologies]

6 hours ago, bfarber said:

So I'm clear, you are saying you have a field hidden by toggles but required, and when you submit it throws an error since no value is provided, even though it's hidden?

Yes.(Not stealing the topic, but I have to chime in...)

 

[CodingJungle]

$form = new \IPS\Helpers\Form;

$input = new \IPS\Helpers\Form\YesNo( 'myname', null, false, [ 'toggles' => 'form_myname2' ] );

$form->add( $input );

$text = new \IPS\Helpers\Form\Text( 'myname2', null, true);

$form->add( $text );

if( $val = $form->values() ){}

this code will fail validation if $text is hidden from not being toggled by $input

$form = new \IPS\Helpers\Form;

$input = new \IPS\Helpers\Form\YesNo( 'myname', null, false, [ 'toggles' => 'myname2_js' ] );

$form->add( $input );

$text = new \IPS\Helpers\Form\Text( 'myname2', null, true, [], null, null, null, 'myname2_js');

$form->add( $text );

if( $val = $form->values() ){}

this code however will work and pass validation if $text is hidden from not being toggled by $input. what i've seen in the suite, where you are toggling, you are applying the $id paramters, so i haven't noticed this in any of your products.

[CodingJungle]

            new \IPS\Helpers\Form\YesNo(
                'babble_rooms_enable_password',
                $this->bit[ 'babble_rooms_enable_password' ],
                false,
                [
                    'togglesOn' => [ 'babble_rooms_password' ],
                    'disabled'  => ( $this->id == 2 or $this->id == 1 ) ? true : false
                ]
            ),
            new \IPS\Helpers\Form\Password(
                'babble_rooms_password',
                $this->password ?: null,
                true,
                [ 'minLength' => 5 ],
                null,
                null,
                null,
                'babble_rooms_password'
            ),

http://screencast.com/t/ItXrvIyz

this is this code and how it works, to illustrate what i'm talking about.

[CodingJungle]

            new \IPS\Helpers\Form\YesNo(
                'babble_rooms_enable_password',
                $this->bit[ 'babble_rooms_enable_password' ],
                false,
                [
                    'togglesOn' => [ 'form_babble_rooms_password' ],
                    'disabled'  => ( $this->id == 2 or $this->id == 1 ) ? true : false
                ]
            ),
            new \IPS\Helpers\Form\Password(
                'babble_rooms_password',
                $this->password ?: null,
                true,
                [ 'minLength' => 5 ]
            ),

http://screencast.com/t/wENWDaQADmx

here is the same code, but allowing the form helper to set the $id to $formname_$elementname, it fails validation.

[teraßyte]

I wasn't even aware that passing an ID allows you to bypass the validation check for required fields if they are toggled off. I always use a custom validation function to setup a variable in order to switch it to FALSE if the toggle is off. Good to know for the future

[bfarber]

Ok, in your example you are not passing the ID and that's why it is failing.

You should not rely on auto-generated IDs - we allow you to explicitly pass the ID, and this is "required" if you want to use toggles. You seem to be trying to work around that, and if that works that's fine, but we aren't likely to make concessions at the code level for this.

In short, I recommend passing the ID parameter. I know what you are getting at (typically the last one you "need" is the required flag, and this means you have to pass 3 or 4 other params with default values just so you can specify the ID) but it really isn't that big a deal ultimately and is how the framework is designed. I maintain that attempting to auto-set the IDs is likely to lead us to trouble and isn't necessary - that's just my personal opinion right now.

(And, FWIW, I do write non-official third party code and plugins, so yes I run into this and am familiar with what you are describing, and I still think it isn't too big a deal).

[CodingJungle]

okay