-
-
Notifications
You must be signed in to change notification settings - Fork 137
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 active date props to header #544
add active date props to header #544
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.
@kbwo Thank you for your pull request!
- Can you add this property to the api documentation ?
isActiveDay
andactiveDate
is confusing. Do you mean highlighting a specific date (Date) or a specific day (0..6) ?
|
@acro5piano |
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 should be the last fix! Thanks for your patient
src/components/CalendarHeader.tsx
Outdated
@@ -46,6 +48,7 @@ function _CalendarHeader<T>({ | |||
<View style={[u['z-10'], u['w-50'], borderColor]} /> | |||
{dateRange.map((date) => { | |||
const _isToday = isToday(date) | |||
const _isActiveDate = activeDate ? isActiveDate(date, activeDate) : _isToday |
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 should be performance improvement: if activeDate
is passed, _isToday
will be calculated but not used.
So, the logic should be like this:
const shouldHighlight = activeDate ? date.isSame(activeDate, 'date') : isToday(date)
return (
Also, please note that
- The var name
shouldHighlight
is more clear - I think the function
isActiveDate
is a bit redundant, writing it inline must be simpler isSame(.., 'date')
is more clear thanisSame(.., 'day')
in this case
Sorry that I couldn't find it at the first time ;)
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 your review. I'll fix it!
@acro5piano fixed according to your review! |
800cd46
to
301f97b
Compare
src/components/CalendarHeader.tsx
Outdated
{dateRange.map((date) => { | ||
const _isToday = isToday(date) | ||
const shouldHighlight = activeDate ? date.isSame(activeDate, 'date') : _isToday | ||
return ( |
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.
To reduce unnecessary function call, please do
{dateRange.map((date) => { | |
const _isToday = isToday(date) | |
const shouldHighlight = activeDate ? date.isSame(activeDate, 'date') : _isToday | |
return ( | |
{dateRange.map((date) => { | |
const shouldHighlight = activeDate ? date.isSame(activeDate, 'date') : isToday(date) | |
return ( |
Thank you for your contribution! |
Released on v2.3.0 |
At first, thank you for great library.
I want new feature to display date of blue style except today, so I added active date props to header on this PR.