-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
[Issue 25] Unopinionated month and year selection support #1106
[Issue 25] Unopinionated month and year selection support #1106
Conversation
This is awesome, and I'll review it a bit more thoroughly today! :) |
src/components/CalendarMonth.jsx
Outdated
@@ -185,7 +194,8 @@ class CalendarMonth extends React.Component { | |||
verticalScrollable && styles.CalendarMonth_caption__verticalScrollable, | |||
)} | |||
> | |||
<strong>{monthTitle}</strong> | |||
{renderCaption && renderCaption({ month, onMonthSelect, onYearSelect })} | |||
{!renderCaption && <strong>{monthTitle}</strong>} |
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.
any reason not to make this be the default function for renderCaption?
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.
Nope, sounds great, I'll update that.
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.
Actually, monthTitle
here is potentially derived from the renderMonth
props, so can't make this explicitly a defaultProp
. But can still certainly clean this bit of code up.
src/components/CalendarMonthGrid.jsx
Outdated
initialMonthSubtraction -= 1; | ||
} | ||
newMonth.set('month', newMonthVal).subtract(initialMonthSubtraction, 'months'); | ||
this.props.onMonthChange(newMonth); |
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.
Please destructure this out, so the “this” value of the function isn’t the props object.
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.
Gotcha 👍
src/components/CalendarMonthGrid.jsx
Outdated
initialMonthSubtraction -= 1; | ||
} | ||
newMonth.set('year', newYearVal).subtract(initialMonthSubtraction, 'months'); | ||
this.props.onYearChange(newMonth); |
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.
Please destructure this out, so the “this” value of the function isn’t the props object.
src/utils/isNextMonth.js
Outdated
import moment from 'moment'; | ||
|
||
export default function isNextMonth(a, b) { | ||
if (!moment.isMoment(a) || !moment.isMoment(b)) return false; |
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.
what non-moment values do you expect in these functions? I’d prefer a throw when it’s an unexpected value.
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'll update this and the other spot, thanks.
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 think this is to allow one or both of a and b to be null (startDate and endDate are sometimes null)
src/utils/isPrevMonth.js
Outdated
import moment from 'moment'; | ||
|
||
export default function isPrevMonth(a, b) { | ||
if (!moment.isMoment(a) || !moment.isMoment(b)) return false; |
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.
Thanks for the review, I'll push an update to address your comments.
src/components/CalendarMonth.jsx
Outdated
@@ -185,7 +194,8 @@ class CalendarMonth extends React.Component { | |||
verticalScrollable && styles.CalendarMonth_caption__verticalScrollable, | |||
)} | |||
> | |||
<strong>{monthTitle}</strong> | |||
{renderCaption && renderCaption({ month, onMonthSelect, onYearSelect })} | |||
{!renderCaption && <strong>{monthTitle}</strong>} |
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.
Nope, sounds great, I'll update that.
src/components/CalendarMonthGrid.jsx
Outdated
initialMonthSubtraction -= 1; | ||
} | ||
newMonth.set('month', newMonthVal).subtract(initialMonthSubtraction, 'months'); | ||
this.props.onMonthChange(newMonth); |
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.
Gotcha 👍
src/utils/isNextMonth.js
Outdated
import moment from 'moment'; | ||
|
||
export default function isNextMonth(a, b) { | ||
if (!moment.isMoment(a) || !moment.isMoment(b)) return false; |
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'll update this and the other spot, thanks.
@ljharb I pushed a change to address a couple of your notes, but after some further review I left the type checks in the utils classes in place. The reason was because all the other similar utility classes, (i.e. https://github.com/airbnb/react-dates/blob/master/src/utils/isAfterDay.js) also do this check, so I think the code and behavior should remain consistent with what's already in place there. Thoughts? |
Looks good; I’ll defer to @majapw about those functions (but consistency is probably better) |
My team is interested in having this PR merged. Can we have an estimate of when is this going to happen? |
+1 on this, we have a team here waiting on this feature to go in, is there an estimate on when this will be merged? |
We would really appreciate a merge as well 🙏 |
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.
omg, i am so sorry for how long it has taken me to review this. @GoGoCarl I can address some of the comments I've made if that'll be easier. I think this is reasonable?
There are def some rough things about the react-dates
architecture that makes this harder than it should be... :x
README.md
Outdated
@@ -147,6 +147,7 @@ numberOfMonths: PropTypes.number, | |||
keepOpenOnDateSelect: PropTypes.bool, | |||
reopenPickerOnClearDates: PropTypes.bool, | |||
renderCalendarInfo: PropTypes.func, | |||
renderCaption: PropTypes.func, |
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.
Can we add what these render functions expect to the README? Something like:
renderCaption: PropTypes.func, // ({ month, onMonthSelect, onYearSelect }) => PropTypes.node,
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.
Certainly! I can also document the callbacks.
renderMonth, | ||
renderCalendarDay, | ||
renderDayContents, | ||
renderCaption, |
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 it weird to have both renderCaption
and renderMonth
? Should we better differentiate between the 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.
I do agree it is a bit weird.
My original thought was to modify the method signature of renderMonth
to be something like renderMonth(month, { onMonthSelect, onYearSelect })
. In order to do that, though, and keep compatible with the current UI, the resulting monthTitle
would need to be wrapped in a <strong>
tag only if it was a string, and I didn't know if adding that type check was the way to go.
So, I went with adding a new function instead mainly to preserve the old functionality of renderMonth
without additional type checks. But if you think it makes more sense to do the above, I'm for it.
Otherwise, we can keep the two separate and having something like renderMonth
and renderMonthElement
, the latter being a rename of renderCaption
that emphasizes the expected return value a bit better and linking the two functions.
I'm open to ideas. :)
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.
Let's leave renderMonth for right now and make the change to renderMonthElement
in a follow-up PR (so as to separate out the breaking aspects)
@@ -204,6 +216,32 @@ class CalendarMonthGrid extends React.Component { | |||
onMonthTransitionEnd(); | |||
} | |||
|
|||
onMonthSelect(currentMonth, newMonthVal) { |
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.
currentMonth
is a momentObject, whereas newMonthVal
is a zero-indexed integer?
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.
Yes; similar to how currentMonth
passes the full momentObject. So, really, currentMonth
is currentDate
. newMonthVal
is a value compatible with momentObject.month(val)
-- which indeed is a zero-indexed integer.
src/components/DayPicker.jsx
Outdated
@@ -419,6 +430,30 @@ class DayPicker extends React.Component { | |||
}); | |||
} | |||
|
|||
onMonthChange(currentMonth) { | |||
// Translation value is a hack to force an invisible transition that |
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.
:(
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.
Very much open to suggestions, but the transition (or something like it) is needed for the re-render.
@@ -456,6 +484,10 @@ export default class DayPickerSingleDateController extends React.Component { | |||
return { currentMonth, visibleDays }; | |||
} | |||
|
|||
setDayPickerRef(ref) { |
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.
Do we use this ref?
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 think this was just added for consistency with other components, but if it's not used, happy to remove it.
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 must be a relic of some old code that never got cleaned up
src/utils/isNextMonth.js
Outdated
import moment from 'moment'; | ||
|
||
export default function isNextMonth(a, b) { | ||
if (!moment.isMoment(a) || !moment.isMoment(b)) return false; |
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 think this is to allow one or both of a and b to be null (startDate and endDate are sometimes null)
src/utils/isNextMonth.js
Outdated
|
||
export default function isNextMonth(a, b) { | ||
if (!moment.isMoment(a) || !moment.isMoment(b)) return false; | ||
return b.isSame(a.clone().add(1, 'month'), 'month'); |
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've found isSame
to be slow even when it's got a 'month' or 'day' or whatever argument. Maybe let's do:
return b.month() === a.clone().add(1, 'month').month();
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.
Only issue there is that a = 2017-01-01, b = 2018-02-01 would return true when it would be false. May not be a thing based on how the function is being called, but could be an issue down the line. To me, isSame
may be slower, but it is safer.
Perhaps, if performance is a concern, checking a.year === b.year and a.month === b.month might help?
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.
good call!
src/utils/isPrevMonth.js
Outdated
|
||
export default function isPrevMonth(a, b) { | ||
if (!moment.isMoment(a) || !moment.isMoment(b)) return false; | ||
return b.isSame(a.clone().subtract(1, 'month'), 'month'); |
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've found isSame
to be slow even when it's got a 'month' or 'day' or whatever argument. Maybe let's do:
return b.month() === a.clone().subtract(1, 'month').month();
test/utils/isNextMonth__spec.js
Outdated
@@ -0,0 +1,32 @@ | |||
import moment from 'moment'; |
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.
isNextMonth_spec.js
(one underscore pls)
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.
Thank you for the review @majapw! I am happy to address the changes to the extent that I can, and leave the rest to you to finalize. Please see my additional comments below.
README.md
Outdated
@@ -147,6 +147,7 @@ numberOfMonths: PropTypes.number, | |||
keepOpenOnDateSelect: PropTypes.bool, | |||
reopenPickerOnClearDates: PropTypes.bool, | |||
renderCalendarInfo: PropTypes.func, | |||
renderCaption: PropTypes.func, |
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.
Certainly! I can also document the callbacks.
renderMonth, | ||
renderCalendarDay, | ||
renderDayContents, | ||
renderCaption, |
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 do agree it is a bit weird.
My original thought was to modify the method signature of renderMonth
to be something like renderMonth(month, { onMonthSelect, onYearSelect })
. In order to do that, though, and keep compatible with the current UI, the resulting monthTitle
would need to be wrapped in a <strong>
tag only if it was a string, and I didn't know if adding that type check was the way to go.
So, I went with adding a new function instead mainly to preserve the old functionality of renderMonth
without additional type checks. But if you think it makes more sense to do the above, I'm for it.
Otherwise, we can keep the two separate and having something like renderMonth
and renderMonthElement
, the latter being a rename of renderCaption
that emphasizes the expected return value a bit better and linking the two functions.
I'm open to ideas. :)
@@ -204,6 +216,32 @@ class CalendarMonthGrid extends React.Component { | |||
onMonthTransitionEnd(); | |||
} | |||
|
|||
onMonthSelect(currentMonth, newMonthVal) { |
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.
Yes; similar to how currentMonth
passes the full momentObject. So, really, currentMonth
is currentDate
. newMonthVal
is a value compatible with momentObject.month(val)
-- which indeed is a zero-indexed integer.
src/components/DayPicker.jsx
Outdated
@@ -419,6 +430,30 @@ class DayPicker extends React.Component { | |||
}); | |||
} | |||
|
|||
onMonthChange(currentMonth) { | |||
// Translation value is a hack to force an invisible transition that |
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.
Very much open to suggestions, but the transition (or something like it) is needed for the re-render.
@@ -456,6 +484,10 @@ export default class DayPickerSingleDateController extends React.Component { | |||
return { currentMonth, visibleDays }; | |||
} | |||
|
|||
setDayPickerRef(ref) { |
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 think this was just added for consistency with other components, but if it's not used, happy to remove it.
src/utils/isNextMonth.js
Outdated
|
||
export default function isNextMonth(a, b) { | ||
if (!moment.isMoment(a) || !moment.isMoment(b)) return false; | ||
return b.isSame(a.clone().add(1, 'month'), 'month'); |
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.
Only issue there is that a = 2017-01-01, b = 2018-02-01 would return true when it would be false. May not be a thing based on how the function is being called, but could be an issue down the line. To me, isSame
may be slower, but it is safer.
Perhaps, if performance is a concern, checking a.year === b.year and a.month === b.month might help?
8460a7e
to
4efde5c
Compare
All sounds good, thanks @majapw! |
\o/ Man i have been slow at getting to this. @GoGoCarl I don't want to put too much work on you randomly 2 months later :) so i'm gonna try to address my comments completely and get this merged in today. I've noticed one small issue when the months change heights which I'll try to address before merging in. :) |
No worries, I'm just excited to see this getting closer to the finish line! If I can help out or debug just let me know, happy to help however I can. |
4efde5c
to
adf2cce
Compare
494787c
to
a759dfb
Compare
Hmm, I'm facing a weird issue where the height animation does not occur between non-neighboring months when using the custom caption. What's weird, is that neighboring months do trigger the animation, even when navigating via the caption. I think it has something to do with the transition time? |
Hi 👋. I have already been using code from this PR as a patch in my app. There is one issue that you should be aware of. The caption is rendered "below" weekdays so in some cases (f.e. when custom components are used for select inputs) the weekdays overlap the content of the caption: The way how the DOM tree looks like make it impossible to just use z-index. The current hacky workaround for me is to hide original weekdays with CSS and render them from const captionRenderer = ({ month, onMonthSelect, onYearSelect }) => {
const weekdays = moment.weekdaysMin();
return (
<div>
<div className="DayPicker_weekHeader">
<ul className="DayPicker_weekHeader_ul">
{weekdays.map(day => (
<li key={day} className="DayPicker_weekHeader_li">
{day}
</li>
))}
</ul>
</div>
// ...
</div>
);
} |
@bkzl YES!! Thank you for mentioning this, I thought there was something else I wanted to flag but was convinced that it was the transition issue so I never thought about it again. I, too, ran into the same issue and solved it the same way. I'll see if I can add a tweak to this that will help, unless you have some ideas on this @majapw |
a759dfb
to
4b72e5f
Compare
Hello! I believe I've fixed the month transition issue. Maybe @GoGoCarl @ljharb, y'all can take a look at my last commit and see if it looks reasonable @bkzl @GoGoCarl I dug into the z-index issue a bit, but was unable to come up with a satisfactory solution. Perhaps, the best bet right now is to merge this in as is and open up a separate issue to track the z-index challenge? |
4b72e5f
to
11c01b2
Compare
Destructure a couple props to remove the `this` reference and tidy up the display call to `renderCaption`.
- add story for daypickerrangecontroller - document callbacks in readme - make month comparison utils more performant
This required a slight change in how the height animation is handled. Namely, the height animation is now always on after the initial render.
8aa1fa8
to
08c997c
Compare
Hi I have downloaded the master branch of the library, as there is no TAG yet to include this PR. In the build process
This piece of code
Must be changed to
Or the build will crash. There is another way of building the app or using this new feature? Thanks in advance |
@sauldeleon it sounds like you’re using an old version of node to do the build; try using node 8 or 10. |
@ljharb thanks for the quick response. You are totally right. Updating Node to the last version did the trick. In addition, there is a warning regarding this PR
EDIT:
on lines 88-89. I can do a PR if you want, but for this maybe its easier for you to do it! EDIT 2: Done. To be reviewed: |
Any plan for next release? excited about this feature |
v17.0.0 coming v. soon (e.g. now :P) |
Is there are any explanation to use this year selection feature, please? |
Hi @ardaplun, you can take a look at https://github.com/airbnb/react-dates/blob/master/stories/DayPickerRangeController.js#L151-L182 to see the custom caption code in action! |
What about animation for onMonthChange? It can be the same one that is used for changing single month |
@stahor posting on a closed PR is probably not the best way to get a discussion going! :) Can you please open an issue? |
Quick follow up to @bkzl's suggestion. You should ensure you get the days of the week in order relevant for the locale by passing
|
@whydna it was |
in case someone stumbles upon here, the source code file has been updated few times, so just look for renderMonthElement property, currently @ https://github.com/airbnb/react-dates/blob/master/stories/DayPickerRangeController.js#L210-L233 |
After tracking #25, which has been plaguing many users, myself included, I hope to have implemented, minimally, a stopgap solution to allow users to traverse months and years on the calendar, based on the excellent work contributed by @jvanderz22.
From the comments in the issue, it seems that the major sticking point for the past few months has been around design/UX. For this proposed solution, there is no design required from the maintainers; the caller is 100% responsible for the rendering.
To achieve this, this PR adds a single prop,
renderCaption
, which is expected to be a function that receives a data object as a param and returns a renderable custom caption (i.e. string, number, node). The data object passed torenderCaption
contains the following parameters:moment
object for the current monthmonth
moment objectmoment
objectHere is some sample code of how this could work with simple select inputs (this sample is also available in the storybook in this PR):
Even if there is a solution later to present a pre-designed interface for selecting months and years, it would still be nice to have something like
renderCaption
to allow users to provide their own custom implementation.As of this writing, this PR passes all tests and is updated to version 16.5.0.