renders dates for milestones in home tab #5581
Conversation
|
@ragesoss I have made the PR for this issue please review it and suggest changes, if any. |
|
This is likely to include bugs related to how weeks in the Timeline get turned into dates. A good solution will involve using the same code to calculate the Timeline dates as to calculate the dates for the Milestone component. In addition to using the same code to calculate dates for both places, it will be useful to test this out manually (and include in the screenshots or video for the PR) by looking at a complex timeline that include empty weeks (weeks with holidays marked for each day where the course normally meets) so that you can see the behavior of when the week numbers don't match up to the actual calendar weeks spanned. |
fillingtothemomo
changed the title
|
|
Hmm... I would check whether blackout dates that are not the first week behave differently, as that's where I would expect bugs to be the most likely. However, I think this still exacerbates the problem of complex date code being used independently in multiple places. Rather than having multiple implementations of the code necessary to calculate the dates, I think the right way to fix this will be to either move the calculations higher up in the render chain so that the same results can be passed to both the Timeline and Milestones components, or extract the necessary code to a single function that accepts the same arguments in each place. (I realize that's a substantial refactor that would require a lot more work than the implementation you've done, but I think that's what is necessary to fix this one properly.) |
|
@ragesoss the checks fails for master too so what changes should I make to my code? |
|
@fillingtothemomo no, if there a test this is failing on your PR that also fails on master, that is probably unrelated. I'm going to try to fix the build on master this week. |
|
@ragesoss is the PR ready to be merged or there are still some changes I should make? |
fillingtothemomo
changed the title
|
@fillingtothemomo per my previous January 18 comment, I'm not satisfied with the way this uses separate code to calculate the dates just for the Milestones component. I think it needs to be done in such a way that the same function and same data gets used for dates on both the Milestones component and for the dates on the relevant parts of the Timeline tab. |
I did change the logic of fetching milestone dates in the second commit , might have missed pinging for review , please review the code and suggest changes. |
| @@ -127,6 +127,14 @@ const CourseDateUtils = { | |||
| const timelineStart = startOfWeek(toDate(course.timeline_start)); | |||
| return differenceInWeeks(timelineStart, courseStart); | |||
| }, | |||
| getWeekStartDate(week, timelineStart) { | |||
There was a problem hiding this comment.
It looks like these functions are not used?
There was a problem hiding this comment.
Yes, these are related to the initial commit and thus redundant. I will remove these.
There was a problem hiding this comment.
Thanks. The issue from my January 18 comment remains: this implements new code to calculate the dates within the Milestone component, rather than refactoring how and where the dates are calculated so that the same calculation can be used for both the Milestones and the Timeline.
There was a problem hiding this comment.
@ragesoss I used the calculating timeline dates code logic in week.jsx
and implemented dateCalc in milestones.jsx, should I just implement a dateCalc function in dateCalculator.jsx and use that to calculate dates for both?
There was a problem hiding this comment.
I'm not sure exactly how my suggestion should be implemented, but I know that it's important to do it in a way where we can be pretty certain that the dates on the Timeline tab and the Milestones component will always be in sync with each other and won't accidentally diverge if code is changed in one place but not another.
There was a problem hiding this comment.
@ragesoss A similar logic is being used in week.jsx as well as empty_week.jsx so a function being implemented as a util would make more sense to be in sync for all files. What do you think?
Remove redundant code
fillingtothemomo
force-pushed
the
milestones-showing-dates#279
branch
from
419f3e4
to
2c9e28a
Compare
|
@ragesoss Please review the new changes. |
| @@ -31,6 +33,12 @@ const Milestones = createReactClass({ | |||
| this.props.allWeeks.map((week) => { | |||
| if (week.empty) return null; | |||
|
|
|||
| const datesStr = DateCalculator.calculateDates( | |||
| this.props.timelineStart, this.props.timelineEnd, this.props.index, { zeroIndexed: false } | |||
There was a problem hiding this comment.
Why is this one zeroIndexed false while the other usage is true?
What this PR does
It addresses the issue #279 Milestones section should show dates in addition to week numbers.
I declared two functions getWeekStartDate and getWeekEndDate in my
app/assets/javascripts/utils/course_date_utils.jsthat gets the start and end date of each week and they are used inapp/assets/javascripts/components/overview/milestones.jsxto render the dates of each milestone.Screenshots
Before:
After:
Open questions and concerns