-
Notifications
You must be signed in to change notification settings - Fork 48
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
PCHR-2527: Make calendar show requests touching different months #2084
PCHR-2527: Make calendar show requests touching different months #2084
Conversation
75fb9dd
to
6bd7437
Compare
@@ -315,9 +314,13 @@ define([ | |||
* @return {Promise} | |||
*/ | |||
function loadMonthLeaveRequests () { | |||
var range = { from: vm.month.days[0].date, |
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 indentation looks weird, please make it,
var range = {
from: vm.month.days[0].date,
to: vm.month.days[vm.month.days.length - 1].date
};
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.
Done.
to_date: { to: vm.month.days[vm.month.days.length - 1].date }, | ||
from_date: range, | ||
to_date: range, | ||
options: { or: [['from_date', 'to_date']] }, |
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.
Here we have double square brackets, please confirm if that is correct?
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.
It is correct. cc @AkA84
to_date: { to: vm.month.days[vm.month.days.length - 1].date }, | ||
from_date: range, | ||
to_date: range, | ||
options: { or: [['from_date', 'to_date']] }, |
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 leave a comment in the code, it might get confusing in future.
var month = controller.month; | ||
var range = { from: month.days[0].date, |
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 indentation looks weird, please fix it as suggested above.
deferred.resolve(); | ||
|
||
return deferred.promise; | ||
$q.resolve(); |
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.
Should not it be return $q.resolve()
?
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.
Pardon.
return day.date === pointerDate.format('YYYY-MM-DD'); | ||
})); | ||
// Ensure that pointerDate is in same month/year that component represents | ||
if (pointerDate.month() === vm.month.index && pointerDate.year() === vm.month.year) { |
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.
Why this change is necessary? Please document 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.
@deb1990 do you suggest to extend the comment or document it in the PR description?
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 comment in the code makes sense, but I am not sure why it was necessary in the context of the ticket, so if you document it in the PR, it will be helpful.
Overview
Currently the Leave Calendar does not reflect leave requests that have start and end dates in different months.
Before
After
Technical Details
Previously, only requests that had the both start and end dates from the specified month are shown.
Now, all requests that have at least one day from the specified months are shown.
Comments