Adding a today modifier to DayPickerRangeController #213
Conversation
| @@ -107,6 +107,7 @@ export default class DayPickerRangeController extends React.Component { | |||
| }; | |||
|
|
|||
| this.isTouchDevice = isTouchDevice(); | |||
| this.today = moment(); | |||
There was a problem hiding this comment.
Whether something is a touch device won't change, but "today" will - imagine a component created at 11:59PM. Let's not cache this value. Maybe it should be a prop that's a function that defaults to returning moment - that way, it can be invoked every render, but can be overridden for tests.
There was a problem hiding this comment.
I think just updating this value in componentWillUpdate will accomplish the same thing and create slightly fewer new moment objects... isToday is going to be called potentially hundreds of times per render.
There was a problem hiding this comment.
Right but if the component isn't updated, yet the day ticks over, then what "today" is won't update. I think it needs to be called at least once per every render.
There was a problem hiding this comment.
But componentWillUpdate does get called on every render, right?
There was a problem hiding this comment.
No, I think it only gets called when the render results in a different DOM.
There was a problem hiding this comment.
Either that, or when different props and state will be rendered - I'm not 100% sure.
There was a problem hiding this comment.
Either that, or when different props and state will be rendered - I'm not 100% sure.
I guess I think this is fine because a hover on a day will update it. or clicking on basically any part of the component. Literally the only issue will be that, if you leave the DateRangePicker open, switch windows, and it becomes the next day, when you come back to it, it won't update the today highlighting until you interact with the component... I think this is okay for the fact that we'll only create a fraction of the moment() objects we would otherwise.
| @@ -223,6 +224,10 @@ export default class DayPickerRangeController extends React.Component { | |||
| return isDayBlocked(day) || isOutsideRange(day) || this.doesNotMeetMinimumNights(day); | |||
| } | |||
|
|
|||
| isToday(day) { | |||
| return day.isSame(this.today, 'day'); | |||
There was a problem hiding this comment.
Can you change this to use the isSameDay helper method? It should already be included in this file I think and it protects slightly better against falsey values
| @@ -107,6 +107,7 @@ export default class DayPickerRangeController extends React.Component { | |||
| }; | |||
|
|
|||
| this.isTouchDevice = isTouchDevice(); | |||
| this.today = moment(); | |||
There was a problem hiding this comment.
I have a concern about the datepicker being open for a while, or someone using it across day boundaries. Let's leave it in the constructor but also maybe update it in componentWillUpdate (reset it as this.today = moment() again there.
|
So this is perfect and will add a |
|
Sweet! Can you rebase and the squash the commits down into one? |
|
Once @ljharb gives his thumbs up, we'll get this in! :) |
fixes #211