Fixes issues with displayFormat prop's default values by using a thunk #98
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
majapw
force-pushed
the
maja-set-moment-defaults-on-instantiation-not-import
branch
from
15201e2
to
bd9f6d8
Compare
ljharb
added
semver-minor: new stuff
ljharb
approved these changes
Sep 27, 2016
| @@ -138,7 +138,9 @@ export default class DateRangePicker extends React.Component { | |||
|
|
|||
| onEndDateChange(endDateString) { | |||
| const { startDate, isOutsideRange, onDatesChange, onFocusChange, displayFormat } = this.props; | |||
| const endDate = toMomentObject(endDateString, displayFormat); | |||
|
|
|||
| const formatString = typeof displayFormat === 'function' ? displayFormat() : displayFormat; | |||
There was a problem hiding this comment.
the propTypes help ensure it's only a string or function, but perhaps it would be more robust (ie, fail faster when it's not a func or string) if this did typeof displayFormat === 'string' ? displayFormat : displayFormat();?
There was a problem hiding this comment.
Okay. I wasn't sure if it made more sense to have it fail abruptly in this way or fail silently, but still function, in the other. I can change it to the former.
There was a problem hiding this comment.
i'm a big fan of failing fast and loud
| @@ -175,7 +177,8 @@ export default class DateRangePicker extends React.Component { | |||
|
|
|||
| onStartDateChange(startDateString) { | |||
| const { displayFormat } = this.props; | |||
| const startDate = toMomentObject(startDateString, displayFormat); | |||
| const formatString = typeof displayFormat === 'function' ? displayFormat() : displayFormat; | |||
There was a problem hiding this comment.
also here. should this logic be consolidated into a helper function?
| @@ -203,8 +206,9 @@ export default class DateRangePicker extends React.Component { | |||
|
|
|||
| getDateString(date) { | |||
| const { displayFormat } = this.props; | |||
| if (date && displayFormat) { | |||
| return date && date.format(displayFormat); | |||
| const formatString = typeof displayFormat === 'function' ? displayFormat() : displayFormat; | |||
| @@ -68,7 +68,8 @@ export default class SingleDatePicker extends React.Component { | |||
|
|
|||
| onChange(dateString) { | |||
| const { displayFormat, isOutsideRange, onDateChange, onFocusChange } = this.props; | |||
| const date = toMomentObject(dateString, displayFormat); | |||
| const formatString = typeof displayFormat === 'function' ? displayFormat() : displayFormat; | |||
| @@ -114,8 +115,9 @@ export default class SingleDatePicker extends React.Component { | |||
|
|
|||
| getDateString(date) { | |||
| const { displayFormat } = this.props; | |||
| if (date && displayFormat) { | |||
| return date && date.format(displayFormat); | |||
| const formatString = typeof displayFormat === 'function' ? displayFormat() : displayFormat; | |||
majapw
force-pushed
the
maja-set-moment-defaults-on-instantiation-not-import
branch
from
bd9f6d8
to
cea7d28
Compare
majapw
deleted the
maja-set-moment-defaults-on-instantiation-not-import
branch
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
to: @ljharb
When attempting to bump the version of
react-datesused on Airbnb, we noticed that the display format for theDateRangePickerandSingleDatePickercomponents was often returning nonsense because moment's locale was not yet set. Similarly, live right now on the storybook, you can see that the format doesn't work at all for the non-english locale example. This fixes this issue.