Skip to content

Conversation

@andrelec1
Copy link
Contributor

@andrelec1 andrelec1 commented Apr 4, 2018

  • Please check if the PR fulfills these requirements
  • The commit message follows our guidelines
  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)
  • What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
    Add simple *1 on value for forcing number type.
  • What is the new behavior (if this is a feature change)?
    Bug fix
  • Does this PR introduce a breaking change? (What changes might users need to make in their application due to this PR?)
    No
  • Other information:

@zoul0813
Copy link
Member

zoul0813 commented Apr 4, 2018

I don't agree with these changes - but am open to being convinced otherwise as I may be missing something here.

NaN is the appropriate value to be used when the user enters anything that is "not a number". I also don't agree that numbers stored as strings '0', '1', '2', etc ... should be treated as anything other than strings. The field expects a number, parseInt('0') or new Number('5') should be used to convert string values into their number representation before passing the model to VFG.

@andrelec1 andrelec1 force-pushed the master branch 2 times, most recently from 2d1cd62 to 31e7ac7 Compare April 4, 2018 16:02
@andrelec1
Copy link
Contributor Author

andrelec1 commented Apr 4, 2018

finaly I found the real issue, and change my commit ...
if(0) => false :/

@zoul0813
Copy link
Member

zoul0813 commented Apr 4, 2018

What’s wrong with the NaN result? Is the browser having an issue displaying NaN in the input?

@andrelec1
Copy link
Contributor Author

andrelec1 commented Apr 4, 2018

in fact, if you put 0 if the field, you didn't convert the value in number ... ( if (0) => false )

onInput($event) {
	let value = $event.target.value;
	switch(this.schema.inputType.toLowerCase()) {
		case "number":
		case "range":
			if($event.target.valueAsNumber) {
				value = $event.target.valueAsNumber;
			}
			break;
	}
	this.value = value;
},

So when value go through formatNumberToModel ( my first wrong fix :/ ) you have a string ... so you put NaN in model ...
In some case you realy want to put 0 in field and get 0 in model ... 0 is valide number ...

https://jsfiddle.net/0mg1v81e/1978/ <= try it , you can't put 0 in field

@qkdreyer
Copy link
Contributor

qkdreyer commented Apr 4, 2018

I can confirm this fixes #425.
You can check it here : https://jsfiddle.net/0mg1v81e/2323/

@zoul0813
Copy link
Member

zoul0813 commented Apr 4, 2018

Not sure I understand what's being fixed here.

Here's the same JSFiddle, pointing to the official 2.1.1 version of VFG ... same result, unless I'm missing something?

I'm able to type in -10, -25, 0, etc ... and the form validates them all, what am I missing here?

@qkdreyer
Copy link
Contributor

qkdreyer commented Apr 4, 2018

Without PR : http://recordit.co/bjcamzQq62
With PR : http://recordit.co/XvuAQjTNBV

@zoul0813
Copy link
Member

zoul0813 commented Apr 4, 2018

Do you get the same result when using my fiddle against 2.1.1?

@qkdreyer
Copy link
Contributor

qkdreyer commented Apr 4, 2018

I'm sorry but I am not able to find your fiddle...

@zoul0813
Copy link
Member

zoul0813 commented Apr 4, 2018

Doh, I didn't link it ... https://jsfiddle.net/94dcctbf/3/

@zoul0813
Copy link
Member

zoul0813 commented Apr 4, 2018

Aha! My fiddle uses 2.1.1, not 2.2.1 ... looks like a bug was introduced somewhere in between. I can confirm this is an issue now.

@qkdreyer
Copy link
Contributor

qkdreyer commented Apr 4, 2018

Yeah just spotted that too !

@qkdreyer
Copy link
Contributor

qkdreyer commented Apr 5, 2018

Could you hotfix/merge this ASAP since it’s a BC minor change and some of us rely on this library in production ?

@zoul0813 zoul0813 merged commit 2cf4a47 into vue-generators:master Apr 16, 2018
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