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

Suggestion getValue() on Select editor to parseInt #428

Open
robert2d opened this issue Nov 4, 2014 · 5 comments
Open

Suggestion getValue() on Select editor to parseInt #428

robert2d opened this issue Nov 4, 2014 · 5 comments

Comments

@robert2d
Copy link
Contributor

robert2d commented Nov 4, 2014

Hi I'd like to make a recommendation to the Select editors getValue() method. At current when using an "id" of a model say, it will return a string from the select list which is fine when using it in conjunction with parseInt for handling parent dependant select lists(like country - region). But having to always parse the value to an Int seems to me like a code smell.

I propose changing the getValue() method to something that will parse an Integer if possible otherwise returning the normal this$el.val(). Which will cut down the need to parseInt when checking a select lists value against say constants for handling other form events like hiding or showing other form inputs.

Thoughts?

getValue: function() {
   if  _.isNaN(parseInt(this.$el.val())) {
    return this.$el.val()
  } else {
    return parseInt(this.$el.val())
  }
},

Referencing: https://github.com/powmedia/backbone-forms/blob/master/src/editors/select.js#L149

@philfreo
Copy link
Collaborator

philfreo commented Nov 4, 2014

There's not really a good way to know if an ID should always be numeric or not, and "sometimes" returning an int vs string seems like a bad idea to me.

@robert2d
Copy link
Contributor Author

robert2d commented Nov 4, 2014

I am only suggesting this. More and more projects are moving to UUID's anyway which are strings. But even then it'd return a string if it was one. Im just wondering if an Int should be parsed more often than not it. I think in most circumstances a developer would be expecting an Int value if it can be parsed as one.. Not a big issue of course but this would save scattering parseInt through my forms

@iabw
Copy link
Contributor

iabw commented Nov 14, 2014

parseInt is very aggressive. It seems way better to subclass Select to do what you need than to make this the default behavior.

It's easy to make your own 'SelectInteger' editor:

Form.editors.SelectInteger = Form.editors.Select.extend({
  getValue: function(){
    var value = this.$el.val(),
        parsed = parseInt( value );
    return _.isNaN( parsed ) ? value : parsed;
  }
});

declared in your model like any other editor:

var schema = {
  "mySelect": {
    "type": "SelectInteger"
  }
}

@robert2d
Copy link
Contributor Author

Fair enough, I rolled my own editor as you suggested but I use it more than
the default editor so thought that may be the same as the community.

Thanks

Dave Robertson
Chief Technology Officer
CricHQ
E: [email protected]
Ph: +64 (0)29 770 9595

On 15/11/2014, at 4:39 am, Ian [email protected] wrote:

parseInt is very aggressive. It seems way better to subclass Select to do
what you need than to make this the default behavior.

It's easy to make your own 'SelectInteger' editor:

Form.editors.SelectInteger = Form.editors.Select.extend({
getValue: function(){
var value = this.$el.val(),
parsed = parseInt( value );
return _.isNaN( parsed ) ? value : parsed;
}
});

declared in your model like any other editor:

var schema = {
"mySelect": {
"type": "SelectInteger"
}
}


Reply to this email directly or view it on GitHub
#428 (comment)
.

@iabw
Copy link
Contributor

iabw commented Nov 15, 2014

It could be a common community requirement. I've never needed selects to produce ints, but I don't currently use a collection as options anywhere.

I see the argument for Selects whose options are a collection returning those values as an int, since it's explicitly using the id, something like -

  getValue: function(){
    var value = this.$el.val(),
        parsed = parseInt( value ),
        isCollection = this.schema.options instanceof Backbone.Collection;
    return isCollection && !_.isNaN( parsed ) ? parsed : value;
  }

but there's still the possibility that this could corrupt your values if the models in your collection had a custom idAttribute like "name" and values like "5 golden rings" or "3-restaurant.com"

To me, it seems much better as a separate class you explicitly use when you know you want ids.

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

No branches or pull requests

3 participants