Skip to content
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

feat(milestones):add dates to milestones section #6020

Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion app/assets/javascripts/components/course/course.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import CourseAlerts from './course_alerts';
import { getStudentCount, getCurrentUser, getWeeksArray } from '../../selectors';
import ActivityHandler from '../activity/activity_handler';
import CourseApproval from './course_approval';
import { getAllWeekDates } from '../../selectors/index';

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove this.

const Course = withRouter((props) => {
useEffect(() => {
Expand Down Expand Up @@ -80,6 +81,7 @@ const Course = withRouter((props) => {
course_id: courseSlug,
current_user: userRoles,
course,
allWeekDates: props.allWeekDates,
};

let courseApprovalForm;
Expand Down Expand Up @@ -170,7 +172,8 @@ const mapStateToProps = state => ({
weeks: getWeeksArray(state),
usersLoaded: state.users.isLoaded,
studentCount: getStudentCount(state),
currentUser: getCurrentUser(state)
currentUser: getCurrentUser(state),
allWeekDates: getAllWeekDates(state)
});

const mapDispatchToProps = {
Expand Down
18 changes: 14 additions & 4 deletions app/assets/javascripts/components/overview/milestones.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import createReactClass from 'create-react-class';
import { filter } from 'lodash-es';

import CourseDateUtils from '../../utils/course_date_utils';
import DateCalculator from '../../utils/date_calculator';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this still needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope,I forgot to remove the DateCalculator. I'll go ahead and remove it now, sorry about that.


const md = require('../../utils/markdown_it.js').default();

Expand All @@ -12,9 +13,11 @@ const Milestones = createReactClass({

propTypes: {
timelineStart: PropTypes.string.isRequired,
timelineEnd: PropTypes.string.isRequired,
weeks: PropTypes.array.isRequired,
allWeeks: PropTypes.array.isRequired,
course: PropTypes.object.isRequired
course: PropTypes.object.isRequired,
weekDates: PropTypes.array.isRequired
},

milestoneBlockType: 2,
Expand All @@ -28,19 +31,26 @@ const Milestones = createReactClass({
const weekNumberOffset = CourseDateUtils.weeksBeforeTimeline(this.props.course);
const blocks = [];

this.props.allWeeks.map((week) => {

this.props.allWeeks.map((week,navIndex) => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where does this come from? In Timeline.jsx, it the result of some complex code in the render cycle for handling empty weeks.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The navIndex is generated by the map function, which helps identify unique elements in the array. Additionally, the DateCalculator requires the navIndex prop for its calculations. Upon cross-checking in the Timeline page, I confirmed that the navIndex is simply the index number generated by the map function.

if (week.empty) return null;

const milestoneBlocks = filter(week.blocks, block => block.kind === this.milestoneBlockType);
return milestoneBlocks.map((block) => {
if (
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks we're doing something similar in both this component and the Timeline, checking for whether to map a particular block to null. Is this something that can become shared logic in the selector, or do they handle these cases differently?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, the problem I encountered is that in the selector, I needed the index for the weeks to display, so I used weeks in the const getWeeks = state => state.timeline.weeks; function to memoize all the results. However, the issue is that the allWeekDates being sent from getAllWeekDates(state) contains weeks that are after the timeline end date, but the getWeeks function doesn't include them.

To handle this, when creating the DateCalculator instance:

const dateCalc = new DateCalculator(course.timeline_start, course.timeline_end, index, {
  zeroIndexed: true
});

I found that the last indexes didn't have any dates, causing it to be empty when trying to render in the milestones and timeline. To fix this, I added a check for that in both files.

!this.props.weekDates[navIndex]?.start ||
!this.props.weekDates[navIndex]?.end ||
this.props.weekDates[navIndex]?.start > this.props.weekDates[navIndex]?.end
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is quite verbose, but it's not obvious what this if statement is for. What situation(s) for a block are being checked for here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This check was added to prevent any date from showing after the timeline's end date in the milestone.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm... I don't think that' necessary. The Timeline can handle dates that are after the end date (in which case, they get highlighted in red). I think in the Milestones component, it would be better to show dates that stretch beyond the end date, when that happens.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I will update the part where the deadline extends. Should I display the extended deadline within the milestone like in the below image, or should I show it after the timeline's end date?
image

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think any special formatting is needed in the Milestones component.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I have updated it. Could you please check it?

) {
return null;
}
let classNames = 'module__data';
if (this.weekIsCompleted(week, currentWeek)) { classNames += ' completed'; }
const rawHtml = md.render(block.content || '');
const completionNote = this.weekIsCompleted(week, currentWeek) ? '- Complete' : undefined;
return blocks.push(
<div key={block.id} className="section-header">
<div className={classNames}>
<p>Week {week.weekNumber + weekNumberOffset} {completionNote}</p>
<p>Week {week.weekNumber + weekNumberOffset} ({this.props.weekDates[navIndex]?.start} - {this.props.weekDates[navIndex]?.end}) {completionNote}</p>
<div className="markdown" dangerouslySetInnerHTML={{ __html: rawHtml }} />
<hr />
</div>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,8 @@ const Overview = createReactClass({
activateValidations: PropTypes.func.isRequired,
firstErrorMessage: PropTypes.string,
isValid: PropTypes.bool.isRequired,
courseCreationNotice: PropTypes.string
courseCreationNotice: PropTypes.string,
allWeekDates: PropTypes.array,
},

componentDidMount() {
Expand Down Expand Up @@ -159,7 +160,7 @@ const Overview = createReactClass({
refetchCourse={this.props.refetchCourse}
/>
<AvailableActions course={course} current_user={this.props.current_user} updateCourse={this.props.updateCourse} courseCreationNotice={this.props.courseCreationNotice} />
<Milestones timelineStart={course.timeline_start} weeks={this.props.weeks} allWeeks={this.props.allWeeks} course={course} />
<Milestones timelineStart={course.timeline_start} timelineEnd={course.timeline_end} weeks={this.props.weeks} allWeeks={this.props.allWeeks} course={course} weekDates={this.props.allWeekDates} />
</div>
) : (
<div className="sidebar" />
Expand Down
17 changes: 13 additions & 4 deletions app/assets/javascripts/components/timeline/timeline.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,8 @@ const Timeline = createReactClass({
editableTitles: PropTypes.bool,
enableReorderable: PropTypes.func,
enableEditTitles: PropTypes.func,
current_user: PropTypes.object
current_user: PropTypes.object,
weekDates: PropTypes.array,
},

getInitialState() {
Expand Down Expand Up @@ -388,16 +389,24 @@ const Timeline = createReactClass({
navClassName += ' is-current';
}

const dateCalc = new DateCalculator(this.props.course.timeline_start, this.props.course.timeline_end, navIndex, { zeroIndexed: true });
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this was the only use of DateCalculator, it no longer needs to be imported in the file.

const navWeekKey = `week-${navIndex}`;
const navWeekLink = `#week-${navIndex + 1 + weeksBeforeTimeline}`;
const startDate = this.props.weekDates[navIndex]?.start;
const endDate = this.props.weekDates[navIndex]?.end;

if (
!startDate ||
!endDate
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why were these checks not necessary in the current implementation?

) {
return null;
}

// if using custom titles, show only titles, otherwise, show default titles and dates
let navItem;
if (usingCustomTitles) {
let navTitle = '';
if (weekInfo.emptyWeek) {
const datesStr = `${dateCalc.start()} - ${dateCalc.end()}`;
const datesStr = `${startDate} - ${endDate}`;
navTitle = I18n.t('timeline.week_number', { number: datesStr });
} else {
navTitle = weekInfo.title ? weekInfo.title : I18n.t('timeline.week_number', { number: navIndex + 1 + weeksBeforeTimeline });
Expand All @@ -411,7 +420,7 @@ const Timeline = createReactClass({
navItem = (
<li className={navClassName} key={navWeekKey}>
<a href={navWeekLink}>{I18n.t('timeline.week_number', { number: navIndex + 1 + weeksBeforeTimeline })}</a>
<span className="pull-right">{dateCalc.start()} - {dateCalc.end()}</span>
<span className="pull-right">{startDate} - {endDate}</span>
</li>
);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,7 @@ const TimelineHandler = (props) => {
nameHasChanged={() => false}
edit_permissions={props.editPermissions}
current_user={props.current_user}
weekDates={props.allWeekDates}
/>
{/* {grading} */}
</div>
Expand All @@ -139,7 +140,8 @@ TimelineHandler.propTypes = {
editableBlockIds: PropTypes.array,
all_training_modules: PropTypes.array,
fetchAllTrainingModules: PropTypes.func.isRequired,
editPermissions: PropTypes.bool.isRequired
editPermissions: PropTypes.bool.isRequired,
allWeekDates: PropTypes.array.isRequired
};

const mapStateToProps = state => ({
Expand Down
19 changes: 19 additions & 0 deletions app/assets/javascripts/selectors/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import { STUDENT_ROLE, INSTRUCTOR_ROLE, ONLINE_VOLUNTEER_ROLE, CAMPUS_VOLUNTEER_
import UserUtils from '../utils/user_utils.js';
import { PageAssessmentGrades } from '../utils/article_finder_language_mappings.js';
import CourseDateUtils from '../utils/course_date_utils.js';
import DateCalculator from '../utils/date_calculator';


const getUsers = state => state.users.users;
Expand Down Expand Up @@ -357,3 +358,21 @@ export const getTicketsById = createSelector(
return tickets.all.reduce((acc, ticket) => ({ ...acc, [ticket.id]: ticket }), {});
}
);


export const getAllWeekDates = createSelector(
[getCourse, getWeeks], (course, weeks) => {
const weekDates = Object.keys(weeks).map((weekId, index) => {
const dateCalc=new DateCalculator(course.timeline_start, course.timeline_end,index,{
zeroIndexed:true
}
)
return {
weekId,
start: dateCalc.start(),
end: dateCalc.end()
};
});
return weekDates;
}
);
Loading