vue-generators / vue-form-generator

:clipboard: A schema-based form generator component for Vue.js
MIT License
2.99k stars 531 forks source link

Lack of ID creates errors in console #397

Open xavs opened 6 years ago

xavs commented 6 years ago

https://github.com/vue-generators/vue-form-generator/blob/1300882b5311e59a00cd799674c5beda9258c13e/src/utils/schema.js#L65

Auto generating ID is not a good idea, specially since things like radio buttons share name. This creates errors. Also,if no label or model, it will create fields with "" id, and if there are many, it will raise errors in the console. Maybe skipping the implicit ID altogether would be better?

dflock commented 6 years ago

We could probably replace that line:

return prefix + (schema.inputName || schema.label || schema.model || "")

with this one:

return prefix + (schema.inputName || schema.label || schema.model || Math.random().toString(36).substring(2, 15))

This will generate a random 11 char string as a last resort, if nothing else if available.

You could also hash the schema object if you wanted something stable - this will get you a stable integer that will be unique if the schema object is unique - but will generate the same number when passed the same schema object:

hashCode () {
    var hash = 0;
    for (var i = 0; i < this.length; i++) {
        var character = this.charCodeAt(i);
        hash = ((hash<<5)-hash)+character;
        hash = hash & hash;
    }
    return Math.abs(hash);
}

...

return prefix + (schema.inputName || schema.label || schema.model || hashCode(schema))
dflock commented 6 years ago

Sorry for the drive-by - I don't currently have time to branch, test, etc...

zoul0813 commented 6 years ago

What if we replaced the slugifyFormID call with a call to Lodash's uniqueId ... I can't think of any reason why the ID's on elements shouldn't be randomly generated. We can provide logical prefixes, to help with debugging ... but, there is no reason for us to generate "known" ID values, if they are being used for styling, you can just use the various options for adding CSS classes to the elements.

We can replace slugifyFormID with something like this:

module.exports.slugifyFormID = function (schema, prefix = "") {
  if(schema.id) {
    return prefix + schema.id;
  }
  return uniqueId(kebabCase(schema.prefix + " " + (schema.inputName || schema.label || schema.model || "")));
}
dflock commented 6 years ago

This is what the lodash uniqueid function actually does:

/**
* Generates a unique ID. If `prefix` is given, the ID is appended to it.
*
* @static
* @since 0.1.0
* @memberOf _
* @category Util
* @param {string} [prefix=''] The value to prefix the ID with.
* @returns {string} Returns the unique ID.
* @example
*
* _.uniqueId('contact_');
* // => 'contact_104'
*
* _.uniqueId();
* // => '105'
*/
function uniqueId(prefix) {
  var id = ++idCounter;
  return toString(prefix) + id;
} 
zoul0813 commented 6 years ago

it's unique to every call ... so if it's called 100+ times, each call generates a unique ID from the previous, which provides us with legal "unique ID" values for HTML elements. We don't need fancy "prefix-h4h3h2h12h1" ID's ...

VFG already relies on Lodash for various things, so reinventing the wheel here seems unnecessary. We can reduce the code in VFG, and eliminate an otherwise cumbersome/clunky looking block of code with a more elegant and simple approach.

zoul0813 commented 5 years ago

468, #559 - added a "unique" flag to getFieldID for things such as radios and checklists, it just appends a lodash uniqueId() value onto the end of the auto-generated ID, and may be useful here.

I believe VFG field ID's should all be uniquely generated, if the user wants them to contain "known" values they can pass the id into the field schema explicitly (for accessing the field through jQuery, etc).