-
Notifications
You must be signed in to change notification settings - Fork 494
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
add multi format support to DateInput #437
add multi format support to DateInput #437
Conversation
Deploy preview for rc-calendar failed. Built with commit 3ff0bf6 https://app.netlify.com/sites/rc-calendar/deploys/5bff611640e2a4716e67e188 |
Codecov Report
@@ Coverage Diff @@
## master #437 +/- ##
=========================================
+ Coverage 88.31% 88.62% +0.3%
=========================================
Files 10 10
Lines 702 712 +10
Branches 192 193 +1
=========================================
+ Hits 620 631 +11
+ Misses 69 68 -1
Partials 13 13
Continue to review full report at Codecov.
|
This pull request introduces 2 alerts when merging 117550f into c92cf57 - view on LGTM.com new alerts:
Comment posted by LGTM.com |
This pull request introduces 1 alert when merging 907c18e into c92cf57 - view on LGTM.com new alerts:
Comment posted by LGTM.com |
This pull request introduces 1 alert when merging 6cdfa36 into c92cf57 - view on LGTM.com new alerts:
Comment posted by LGTM.com |
Yay thank you for this 😃 , was just about to try and hack around it 🛠️ are there any blocks to this being merged? |
Cool! I've never seen a bot that has starred 41 repos has 62 personal repos and has a github.io blog... @onlyann thanks for making the two PRs mentioned, do you have write access or do you know who does? |
I don't know the maintainers. We can only hope that one of them will find the time to review and accept the PR |
Aha, I'm not robot but LGTM add auto comment. Currently lack of time, sorry for delay~ |
Pls add the |
I didn't know there was a real account on top of the automated comments 😅 Made the recommended changes. |
str: selectedValue && selectedValue.format(nextProps.format) || '', | ||
invalid: false, | ||
}); | ||
if (!this.state.hasFocus) { |
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 makes str
& invalid
un-control. It's better to compare selectedValue
& format
changed to setState instead of focus status.
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 am not sure I understand what you suggest.
This is done to prevent re-formatting the input while a user is editing the date, i.e. to prevent reformatting 1/12/18
to 01/12/2018
until focus is lost.
I could change the condition to be:
const hasMultipleFormats = !!nextProps.format && nextProps.format.length > 1;
if (!hasMultipleFormats || !this.state.hasFocus) {
this.setState({
str: formatDate(selectedValue, nextProps.format),
invalid: false,
});
}
Would that work for you or am I missing something else?
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 mean when props of selectedValue
or format
changed. Current code will not makes state update. I suggest to modify like this:
if (
!shallowEqual(this.props.format, format) ||
(!this.props.selectedValue && selectedValue) ||
!this.props.selectedValue.isSame(selectedValue, 'date')
) {
// ...
}
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 won't work because on every key stroke, if the input parses to a valid date, the Calendar component will change the selectedValue
prop, effectively triggering componentWillReceiveProps
on DateInput and that will override the date input to a different format while the user is typing.
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.
tested. You're correct.
Extend the
format
prop of theDateInput
to allow an array of formats.The component then uses the array of formats when attempting to parse a date from the input.
The first value of the array determines the displayed format.
A typical use case is to allow the user to enter in several supported formats, such as D/M/YY or DD/MM/YYYY