Feature/numeric input #17
Conversation
kamilmateusiak
requested review from
marcinsawicki,
mjurkowski,
dprzybylek,
lwojciechowski and
jdrzejb
| }; | ||
|
|
||
| calcValue(val) { | ||
| const { max, min } = this.props; |
There was a problem hiding this comment.
this operation is often called clamp:
const clamp = (min, max, v) => Math.min(Math.max(v, min), max)
| [`${baseClass}__increment--disabled`]: | ||
| this.props.max && this.props.value === this.props.max | ||
| })} | ||
| onClick={this.changeValue(1)} |
There was a problem hiding this comment.
cursor: not-allowed; doesnt block click events so u can still click on this and have onChange being fired (it shouldnt be if we are already having the max/min value entered)
| <input | ||
| type="text" | ||
| {...restProps} | ||
| value={value !== null ? value : ''} |
There was a problem hiding this comment.
this component should always be controlled internally or externally - handleChange should either call onChange if its controlled externally or setState if its controlled internally (so uncontrolled externally), otherwise for uncontrolled components it will leave garbage types into the field (u purify the event.target.value, but u dont set it)
| } | ||
|
|
||
| onFocus = () => { | ||
| this.addKeyboardEventListeners(); |
There was a problem hiding this comment.
why those arent handled by the input component?
| return ( | ||
| <span className={mergedClassNames} style={this.getComponentStyles()}> | ||
| <input | ||
| type="text" |
There was a problem hiding this comment.
| value={value !== null ? value : ''} | ||
| disabled={disabled} | ||
| onChange={this.handleChange} | ||
| onFocus={this.onFocus} |
There was a problem hiding this comment.
use one convention for naming event handler functions.
| }; | ||
|
|
||
| onKeyDown = e => { | ||
| if (e.keyCode === KeyCodes.arrowDown) { |
There was a problem hiding this comment.
If you use field type number, all of this is handled by deafult
There was a problem hiding this comment.
Also min/max values are handled by browser
There was a problem hiding this comment.
imho browser validation is insufficient in this case
| }; | ||
|
|
||
| handleChange = e => { | ||
| e.preventDefault(); |
There was a problem hiding this comment.
Why do you need to prevent default action from firing?
| ```js | ||
| initialState = { value: 1, error: null }; | ||
|
|
||
| onInputChange = (value) => { |
| line-height: 36px; | ||
| height: 100%; | ||
| border-radius: 4px; | ||
| border: 1px solid #bcc6d0; |
No description provided.