Conversation
akademy
left a comment
akademy
left a comment
There was a problem hiding this comment.
This looks great. Just a few minor suggestions.
| <> | ||
| { |
| onKeyDown={handleKeyDown} | ||
| onBlur={handleBlur} | ||
| error={!isValid} | ||
| helperText={!isValid ? "Invalid input" : helperText} |
There was a problem hiding this comment.
Can we have a different message when the values are outside the min / max value, it's just slighly easier for users to understand. "Outside of range" or " too high", something like that.
There was a problem hiding this comment.
Helper text for invalid input due to limits is now "Outside limits"
| !defaultValue ? "" : defaultValue.toString(), | ||
| ); | ||
| const [isValid, setIsValid] = useState( | ||
| !defaultValue ? true : Modes[numberMode].test(defaultValue.toString()), |
There was a problem hiding this comment.
Do we want to check the limits here too?
There was a problem hiding this comment.
Limits are now checked at all relevent levels, and corresponding test has been added
| }) => { | ||
| const numberRegex = Modes[numberMode]; | ||
|
|
||
| const helperText = `A ${numberMode} number. Limits: ${minValue} to ${maxValue}`; |
There was a problem hiding this comment.
Can you make it optional to display this helper text?
There was a problem hiding this comment.
Helper text is default, but can now be turned off
| }; | ||
|
|
||
| const handleInputChange = (value: string) => { | ||
| setIsValid(numberRegex.test(value) && checkLimits(value)); |
There was a problem hiding this comment.
Could set different error message at checkLimits level.
| value={numberText} | ||
| onChange={(e) => handleInputChange(e.target.value)} | ||
| onKeyDown={handleKeyDown} | ||
| onBlur={handleBlur} |
There was a problem hiding this comment.
If you do not set either commitOnBlur or commitOnReturn to false and hit return you get in a loop. Enter -> Blur
There was a problem hiding this comment.
This loop seems to have only been an issue due to alert stealing focus from the original element. I've now added logic to blur the element if commitOnBlur is true (thus using the onBlur action to trigger the handleCommit when the enter key is pressed), otherwise handleCommit for onKeyDown happens as normal.
I tried it out a few times without managing to trigger a loop, but let me know if this doesn't work.
3 participants
Fixes #154.
Adds a number input component, which validates various number types, high limits, and low limits.