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

Added option: "typeUserAttrs". #247

Merged
merged 5 commits into from
Sep 16, 2016
Merged

Added option: "typeUserAttrs". #247

merged 5 commits into from
Sep 16, 2016

Conversation

gimigliano
Copy link
Contributor

Suppose you want to add "min" and "max" to all "date" fields and moreover you want to add a parameter "columns" from 1 to 6 to all the "radio-group"

typeUserAttrs:{
'date':{
'min':{'label':'Date min.','maxlength':'10','description':'Minimum'},
'max':{'label':'Date max.','maxlength':'10','description':'Maximum'}
},
'radio-group':{
'columns':{'label':'Column number', 'options':{1:'one',2:'two',3:'three'}
}
},

};

//+ gimigliano
if(opts.typeUserAttrs[values.type]) {}
Copy link
Owner

Choose a reason for hiding this comment

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

is this meant to wrap until L768?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes I inserted only three blocks

Copy link
Owner

Choose a reason for hiding this comment

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

Right but i meant that the conditional starts and ends on L735 because it has {}. Once thats fixed we can merge.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No are 3 differents blocks don't see this page but find them in de .js file. It's more easy

@kevinchappell
Copy link
Owner

I've been testing the last couple hours and seems to work well. In the future we may want to add support for boolean type attributes such as disabled or readonly but I think this is a great start.

After fixing the conditional on L735 I can merge and will add documentation shortly after. Is it okay to use the PR description for documentation and demo?

@gimigliano
Copy link
Contributor Author

gimigliano commented Sep 15, 2016

In today version yo can adda all the attributes you want they will be used without control (so disabled readonly onchange etc. too).
Eg.

typeUserAttrs:{
'radio-group':{
                    'pubdate':{'label':'Pubblication date',  
                                    'type':'date', 
                                    'min':'2016-01-01' , 
                                    'style':'color:red'
                                   } 
                 }

@kevinchappell
Copy link
Owner

Looking good, but do options still work? I pulled the changes but now my previous config does not work. Example:

'radio-group':{
  'columns': {
    'label': 'Column number', 
    'options': {
      1: 'one',
      2: 'two',
      3: 'three'
    }
}

@gimigliano
Copy link
Contributor Author

gimigliano commented Sep 15, 2016

Yes, if options is present the attribute is forced as a "select"

'radio-group':{
 'columns': {
    'label': 'Column number', 
    'options': { //applied
        1: 'one',
        2: 'two',
        3: 'three'
        },
 'type': 'text', //ignored
 'onchange':'alert(1)', //applied
 'title':'Columns number' //applied
}

Copy link
Owner

@kevinchappell kevinchappell left a comment

Choose a reason for hiding this comment

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

looking and working good. needs that one final change and should be good to go.

for (var val in options) if(val!='options' && val!='type' && val!='value' && options[val]!=undefined) selectOpen+=val+'="'+options[val].replace('"','"')+'"';
return '<div class="form-group ' + name + '-wrap">'
+'<label for="' + name + '-' + lastID + '">'+opts.messages[name] +'</label>'
+selectOpen
Copy link
Owner

Choose a reason for hiding this comment

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

To get the select properly i had to change this:

+'<' + selectOpen + '>'

I just saw your correction here but markup is still partially malformed and causes first option to not show.

@gimigliano
Copy link
Contributor Author

Yes now is correct but I saw the select built correctly on firefox ... I hope now is closed

@kevinchappell
Copy link
Owner

Working good for me now. Good work on this, extremely useful feature.

@kevinchappell kevinchappell merged commit 3e7230f into kevinchappell:master Sep 16, 2016
@kevinchappell
Copy link
Owner

Documentation and demo of this feature in action has been added. http://formbuilder.readthedocs.io/en/latest/formBuilder/options/typeUserAttrs/

@eradin
Copy link

eradin commented Sep 19, 2016

Nice feature. I thought it might come in handy for my validation problem but it doesn't. I'm using parsley.js and I want to validate a telephone number (US style) on the rendered form. There is no telephone sub type for a text field. Although, with parsley, I can add a data attribute to the generic text field (data-parsley-phone="true") and it will validate the field for the proper phone format. Unfortunately, there is no way to tack on user supplied attributes using this new option. It would be nice to have a field (e.g. attributes?) similar to the class field that would accept a string verbatim and tack that string onto the input tag. Saving and loading a form would also need to preserve those attributes.

@kevinchappell
Copy link
Owner

I was going to say you could use a pattern attribute similar to here but I saw it gets escaped so looks like some work is needed there.

Edit: i just pushed a fix so attributes should be escaped correctly. Check this pen to see it in action but you may need to wait a bit since gh-pages seems to be a little slow today.

I will also add a phone subtype in the coming days.

@eradin
Copy link

eradin commented Sep 20, 2016

Thanks Kevin. The pattern doesn't solve my problem but adding tel as a sub type would (just tested it). If you are going to add tel, consider adding number also. These are really just text types but they allow mobile browsers to bring up the alternative keyboard. Cheers.

@kevinchappell
Copy link
Owner

@eradin The phone pattern is working for me locally but for some reason gh-pages is still serving v1.18.0 instead of v1.19.2. I understand it may not suit your use case though.

Can you describe the number input? Is it just <input type="number">? That should already be enabled by default. Originally it was disabled but about a month ago it was made enabled by default.

I'll add tel now.

@eradin
Copy link

eradin commented Sep 20, 2016

http://formbuilder.online/assets/js/form-builder.min.js is serving up 1.18.
Scratch the number type request. I see it on the pallet. I didn't notice it before. tel probably only needs to be a sub-type of text.

@kevinchappell
Copy link
Owner

I just checked https://formbuilder.online/assets/js/form-builder.min.js and it seems to be serving 1.19.3 now. May need to clear cache. 1.19.3 has tel subtype of text

@eradin
Copy link

eradin commented Sep 20, 2016

Yup, that was the problem. Looks real good. Not to be picky but is there a simple way for you to make the dropdown say phone or telephone but insert a tel type? I'm afraid users won't get the short form name.

@kevinchappell
Copy link
Owner

That makes sense but would require a bit of refactoring to make happen. I'll look into it though when I get the chance.

@dinevillar
Copy link

dinevillar commented Oct 6, 2016

This feature have errors on the textUserAttrs function when you try to add a checkbox as a custom attribute field. The 'form-control' class added to the checkbox destroys its look and when you try to add a field on the defaultFields with this custom attribute set to true/false it is not setting the value. Anyway, I made a fix myself:

function textUserAttrs(name, options) {
      var textAttrs = {
        id: name + '-' + lastID,
        title: options.description || options.label || name.toUpperCase(),
        name: name,
        type: options.type || 'text',
        className: 'fld-' + name + (options.type === 'checkbox' ? '' : ' form-control')
      },
          label = '<label for="' + textAttrs.id + '">' + opts.messages[name] + '</label>';
      if (options.type === 'checkbox' && options.value) {
        textAttrs.checked = 'checked';
      }
      textAttrs = Object.assign({}, options, textAttrs);
      var textInput = '<input ' + utils.attrString(textAttrs) + '>';
      return '<div class="form-group ' + name + '-wrap">' + label + textInput + '</div>';
    }

@kevinchappell
Copy link
Owner

@dinevillar good catch on that.

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.

4 participants