-
Notifications
You must be signed in to change notification settings - Fork 15
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
InputNumber extends AbstractNumberField #26
base: master
Are you sure you want to change the base?
Conversation
Looks good. Added a spec should be pretty easy :) |
value = "10"; | ||
field.value = value; | ||
|
||
expect(field.value).toBe(value); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So we're not converting the input string to be a number?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Part of me wonders if it should be up to the end user to do the conversion manually, or with a converter, passing anything other than a number would simply put NaN
as the value of the field. Thoughts?
i.e I don't think I'd expect a date-input alone to do the conversion from a string to a rich date object.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The super class does have a converter
property for the user: https://github.com/montagejs/native/blob/363aad000079eae2b7ead4721966a0319e24fefc/ui/text-input.js#L100
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dates are more complex than numbers. if we put type=number on an input, this kind of data validation (preventing the user from entering "foo" in the first place) seems like a good idea.
i added a test to show that it wont give NaN
, but rather reject the users' input and set the value to ""
ok @cesine, I think I was misreading this as I assumed the number input was based on abstractControls. With it still being based on top of native we're in this weird grey area when we try to make it behave less like the nave So maybe a new task/extension of this one: can you make the matte number input a specialization of the corresponding abstract control? bringing this functionality in line as part of that transition? |
instead of native input, now value is forced to be an int or float (a number) AbstractNumberField requires plus and minus buttons, but this commit uses the input type=number's built in plus and minus buttons.
needs css work to get the buttons inline
@@ -0,0 +1,12 @@ | |||
.matte-InputNumber { | |||
display: inline-block; | |||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The css needs work.
This inline-block css has no effect, the controls are stacked...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@simurai i tried a few things to get the buttons to be inline but they don't fit... i will try again today.
with no side effects on input text
with montage b106a084ad36721211fcd503ab97d63a83b026b3
updated tests, all pass except converting international number formats
var field = testPage.test.valueless; | ||
|
||
runs(function(){ | ||
expect(field.checkValidity()).toBe(false); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this was field.element.checkValidity() but I couldnt find if it should be on element intentionally
fixes problems with
<input type='number'>
returning a string and adds the other functionality enjoyed by AbstractControl instead of NativeControl.