Skip to content

Lack of ID creates errors in console #397

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

Open
xavs opened this issue Feb 19, 2018 · 6 comments
Open

Lack of ID creates errors in console #397

xavs opened this issue Feb 19, 2018 · 6 comments

Comments

@xavs
Copy link

xavs commented Feb 19, 2018

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

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
Copy link
Collaborator

dflock commented Feb 19, 2018

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
Copy link
Collaborator

dflock commented Feb 19, 2018

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

@zoul0813
Copy link
Member

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
Copy link
Collaborator

dflock commented Mar 20, 2018

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
Copy link
Member

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
Copy link
Member

#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).

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

No branches or pull requests

3 participants