Resolution of issues #468 and possibly #483#551
Resolution of issues #468 and possibly #483#551ErvinSabic wants to merge 5 commits intoCrowdStrike:mainfrom
Conversation
|
simonihmig
left a comment
simonihmig
left a comment
There was a problem hiding this comment.
Looking good, thanks @ErvinSabic! But we would need some test for this. @averydev had one already here, linked in #468.
Also just merged #550, so if you could rebase this should give us a green build (ignoring the known docs deployment issue)
| } | ||
| // This is here to avoid setting the value as NaN, instead using Zero. | ||
| else if(e.target.value === "" || isNaN(valueAsNumber)){ | ||
| this.args.setValue(0); |
There was a problem hiding this comment.
I am not quite sure about this one. Can you explain your rationale for this case?
Wouldn't not setting the value here be more appropriate?
There was a problem hiding this comment.
Here's a video explaining my thinking -
zerodefault.mp4
There was a problem hiding this comment.
Defaulting to a zero is simpler than maintaining the last valid state like I do on my intl number input. Number inputs should always be numbers.
add test for issue with entering numbers with a decimal
…aned input component
|
I just went ahead and pulled the latest from the base branch, added a variation of the test you'd given along with an extra. Should test and merge in green now. |
|
Something seems to be going on with prettier in the linting process? |
3 participants
Used some of @simonihmig 's suggestions along with accounted for NaN or blank values defaulting to zero.