-
Notifications
You must be signed in to change notification settings - Fork 26
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
refactor(date-input): use hooks #963
Conversation
inputRef.current.setSelectionRange(0, 100); | ||
emit(date); | ||
}, | ||
[inputRef] |
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.
I'm not sure if this makes sense 🤔
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.
To my understanding yes. useCallback does not have smart exceptions like any value of props or any ref. It is just a closure and memoization as far as I understood it.
this.props.value === '' ? null : getDateInMonth(this.props.value) - 1, | ||
}; | ||
|
||
showPrevMonth = () => { |
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 never used
const nextDate = changeMonth(calendarDate, amount); | ||
setCalendarDate(nextDate); | ||
setHighlightedIndex(0); | ||
}; |
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.
Is there a specific reason to not useCallback these two?
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 looks good. Thanks.
ef38b1a
to
6cefdef
Compare
calendarDate: today, | ||
highlightedIndex: | ||
prevState.suggestedItems.length + getDateInMonth(today) - 1, | ||
const { onChange } = props; |
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.
Same here
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.
Yeah, this is how I would go about it too. Thanks.
}, | ||
}); | ||
}; | ||
}, [onBlur, props.id, props.name]); |
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.
And here
Summary
Refactors the
DateInput
component to use hooks.