-
-
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
Calendar caption with renderCalendarCaption #341
Calendar caption with renderCalendarCaption #341
Conversation
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.
Hi! Sorry for the delay in review. :) This looks great! I have one small nit, and then some questions about the naming of the prop in general. I'd love to get this in ASAP though.
README.md
Outdated
@@ -136,6 +136,11 @@ By default, we do not show days from the previous month and the next month in th | |||
initialVisibleMonth: PropTypes.func, | |||
``` | |||
|
|||
Optionally you can provide calendar caption block renderer. | |||
```js | |||
renderCalendarCaption: 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.
I'm concerned with the nomenclature of using caption
here as we already have a <caption>
dom element on each table inside each CalendarMonth
component. I think using Calendar
is also somewhat problematic as this is rendered on the DayPicker
level not on the CalendarMonth
level. Perhaps renaming it to renderDayPickerCaption
or renderDayPickerAdditionalInfo
might be a good idea? What do you think?
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.
Oh this is totes my fault by having the Calendar props
story isn't it. What if we called this renderCalendarAdditionalInfo
or something? Maybe even just renderAdditionalInfo
? God, I'm verbose.
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.
@majapw Glad to see your response!
Maybe its better to this props renderCalendarInfo
?
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. Let's do that instead!
test/components/DayPicker_spec.jsx
Outdated
@@ -64,6 +64,15 @@ describe('DayPicker', () => { | |||
}); | |||
}); | |||
|
|||
describe('renderCaption', () => { | |||
it('caption exists', () => { | |||
const testCaptionClass = 'test-cation-container'; |
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.
nit: test-caption-container
5e38765
to
570a8f1
Compare
570a8f1
to
864c302
Compare
An excerpt from README:
Usage example:
Example screenshot:
The
renderCalendarInfo
prop are universal, and available inDateRangePicker
,SingleDatePicker
andDayPicker
.