-
Notifications
You must be signed in to change notification settings - Fork 943
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
Show available time slots in date input #873
Conversation
@@ -497,7 +497,7 @@ ListingPageComponent.defaultProps = { | |||
showListingError: null, | |||
reviews: [], | |||
fetchReviewsError: null, | |||
timeSlots: [], | |||
timeSlots: null, |
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.
The default timeSlots
value on listing page level is changed to null
so that it can be used to resolve if time slots are used. An empty array that would be passed to subcomponents would just render a blocked calendar.
@@ -151,6 +157,12 @@ class DateInputComponent extends Component { | |||
|
|||
const date = value && value.date instanceof Date ? moment(value.date) : initialMoment; | |||
|
|||
const isDayBlocked = timeSlots | |||
? day => { | |||
return !timeSlots.find(timeSlot => isSameDay(day, moment(timeSlot.attributes.start))); |
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 the timeSlot.attributes.start
always a start of a single day or could it be a range of days?
Either way, it could be good to have a short comment about response restrictions and rules?
Another related response format question: is the returned day in UTC 00:00 timestamp or has the timezone difference added to it already?
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.
- Time zones need some conversions or comments.
- Are time slots always single days and not date ranges? (I think that was how it was defined, but it wouldn't hurt to have a comment here as well - since timeslots are going to be expanded later to weeks / hours / etc. - it probably would be good to check timeslot type too.)
Also adds an extra example so that there's one with and without time slots.
@Gnito addressed the comments, this is up for a new review. |
// the start date is used to represent available dates. | ||
const localStartDate = dateFromAPIToLocalNoon(timeSlot.attributes.start); | ||
|
||
return isSameDay(day, moment(localStartDate)); |
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.
Add also a check for type: "time-slot/day"
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.
Something like ensureTimeSlot(timeSlot).attributes.type === 'time-slot/day'
id: new UUID(i), | ||
type: 'timeSlot', | ||
attributes: { | ||
start: moment(startTime) |
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 needs to use UTC date - otherwise, customizers get errors if they run this (ie. their laptop) in different timezones than UTC.
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.
Hmm. This is only called from styleguide - so, it's not needed for test verification. I'm not sure if this will cause problems or not. (I was assuming that startDate should be something like dateFromLocalToAPI(startDate)
, but moment's startOf
function doesn't know about UTC start of day moment anyway...)
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.
Two more comments - otherwise, this looks good.
Also fixes time slots test data creation by setting the date time correctly to UTC.
return isSameDay(day, moment(localStartDate)); | ||
} else { | ||
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.
This works but it could be written shorter as
const isDay = ensureTimeSlot(timeSlot).attributes.type === TIME_SLOT_DAY;
return isDay && isSameDay(day, moment(localStartDate))
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.
Looks elegant. One drawback though is that the localStartDate
needs to be resolved with dateFromAPItoLocalNoon
even though the time slot type wasn't of type day. But the dateFromAPIToLocalNoon
should handle anything you throw at it.
Blocked dates use the same styling as the out of range ones.