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.

Sign up for GitHub

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
merged 14 commits into from
Dec 13, 2024
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
473d579
feat(milestones):add dates to milestones section
shishiro26 Oct 29, 2024
e1815fe
Merge branch 'WikiEducationFoundation:master' into feature/#279_add_d…
shishiro26 Nov 2, 2024
e0e689e
Refactored code to use the selector and removed unnecessary coupling
shishiro26 Nov 7, 2024
e4795b2
Removed the unnecessary 'DateCalculator' import
shishiro26 Nov 7, 2024
a4f6784
updated the dates which are extending the end date
shishiro26 Nov 8, 2024
96043ea
Merge branch 'WikiEducationFoundation:master' into feature/#279_add_d…
shishiro26 Nov 8, 2024
2efce7d
fixed all the linting errors
shishiro26 Nov 8, 2024
e0693bd
Merge branch 'WikiEducationFoundation:master' into feature/#279_add_d…
shishiro26 Nov 15, 2024
bb4f8b3
fixed the ci errors
shishiro26 Nov 15, 2024
f44f44e
refactor: remove unused weekDates prop and update related components …
shishiro26 Dec 3, 2024
2572a9b
added the weekDates in the else block
shishiro26 Dec 5, 2024
034bfde
Merge branch 'WikiEducationFoundation:master' into feature/#279_add_d…
shishiro26 Dec 9, 2024
e644c96
fixed all the formatting changes
shishiro26 Dec 11, 2024
9174b5c
Merge branch 'WikiEducationFoundation:master' into feature/#279_add_d…
shishiro26 Dec 11, 2024
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
8 changes: 5 additions & 3 deletions app/assets/javascripts/components/overview/milestones.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,8 @@ const Milestones = createReactClass({
timelineStart: PropTypes.string.isRequired,
weeks: PropTypes.array.isRequired,
allWeeks: PropTypes.array.isRequired,
course: PropTypes.object.isRequired
course: PropTypes.object.isRequired,
allWeeksDates: PropTypes.array.isRequired,
},

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

this.props.allWeeks.map((week) => {
this.props.allWeeks.map((week, navIndex) => {
if (week.empty) return null;
const weekDates = this.props.allWeeksDates;

const milestoneBlocks = filter(week.blocks, block => block.kind === this.milestoneBlockType);
return milestoneBlocks.map((block) => {
Expand All @@ -40,7 +42,7 @@ const Milestones = createReactClass({
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} ({weekDates[navIndex].start} - {weekDates[navIndex].end}) {completionNote}</p>
<div className="markdown" dangerouslySetInnerHTML={{ __html: rawHtml }} />
<hr />
</div>
Expand Down
8 changes: 5 additions & 3 deletions app/assets/javascripts/components/overview/overview_handler.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ import { deleteCourse, updateCourse, resetCourse, persistCourse, nameHasChanged,
import { fetchOnboardingAlert } from '../../actions/course_alert_actions';
import { fetchTags } from '../../actions/tag_actions';
import { addValidation, setValid, setInvalid, activateValidations } from '../../actions/validation_actions';
import { getStudentUsers, getWeeksArray, getAllWeeksArray, firstValidationErrorMessage, isValid } from '../../selectors';
import { getStudentUsers, getWeeksArray, getAllWeeksArray, firstValidationErrorMessage, isValid, getAllWeekDates } from '../../selectors';
import OverviewStatsTabs from '../common/overview_stats_tabs';

const Overview = createReactClass({
Expand All @@ -44,12 +44,13 @@ const Overview = createReactClass({
updateClonedCourse: PropTypes.func.isRequired,
weeks: PropTypes.array.isRequired,
allWeeks: PropTypes.array.isRequired,
allWeeksDates: PropTypes.array.isRequired,
setValid: PropTypes.func.isRequired,
setInvalid: PropTypes.func.isRequired,
activateValidations: PropTypes.func.isRequired,
firstErrorMessage: PropTypes.string,
isValid: PropTypes.bool.isRequired,
courseCreationNotice: PropTypes.string
courseCreationNotice: PropTypes.string,
},

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} weeks={this.props.weeks} allWeeks={this.props.allWeeks} course={course} allWeeksDates={this.props.allWeeksDates} />
</div>
) : (
<div className="sidebar" />
Expand Down Expand Up @@ -192,6 +193,7 @@ const mapStateToProps = state => ({
campaigns: state.campaigns.campaigns,
weeks: getWeeksArray(state),
allWeeks: getAllWeeksArray(state),
allWeeksDates: getAllWeekDates(state),
loading: state.timeline.loading || state.course.loading,
firstErrorMessage: firstValidationErrorMessage(state),
isValid: isValid(state),
Expand Down
9 changes: 4 additions & 5 deletions app/assets/javascripts/components/timeline/timeline.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@ import CourseLink from '../common/course_link.jsx';
import Affix from '../common/affix.jsx';
import EditableRedux from '../high_order/editable_redux';

import DateCalculator from '../../utils/date_calculator.js';
import CourseUtils from '../../utils/course_utils.js';
import CourseDateUtils from '../../utils/course_date_utils.js';
import { toDate } from '../../utils/date_utils.js';
Expand All @@ -26,6 +25,7 @@ const Timeline = createReactClass({
course: PropTypes.object.isRequired,
weeks: PropTypes.array,
allWeeks: PropTypes.array,
allWeeksDates: PropTypes.array,
week_meetings: PropTypes.array,
editableBlockIds: PropTypes.array,
editable: PropTypes.bool,
Expand Down Expand Up @@ -387,8 +387,7 @@ const Timeline = createReactClass({
if (navIndex === 0) {
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 weekDates = this.props.allWeeksDates;
const navWeekKey = `week-${navIndex}`;
const navWeekLink = `#week-${navIndex + 1 + weeksBeforeTimeline}`;

Expand All @@ -397,7 +396,7 @@ const Timeline = createReactClass({
if (usingCustomTitles) {
let navTitle = '';
if (weekInfo.emptyWeek) {
const datesStr = `${dateCalc.start()} - ${dateCalc.end()}`;
const datesStr = `${weekDates[navIndex].start} - ${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.

I note that his branch of code is being switched to use weekDates, but another branch below uses dateCalc. Is that the intended/correct behavior?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for pointing this out! I don't recall the exact branch in question, but this is the updated and correct code. The change to weekDates is intentional and reflects the latest update to the logic.

Copy link
Member

Choose a reason for hiding this comment

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

What I mean is, is it intention to switch from dateCalc to weekDates in only one branch, or should it be done in both?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It should be implemented in only one branch, not both.

Copy link
Member

Choose a reason for hiding this comment

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

Okay. Why?

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 other branch is the older one, and I have been working on different changes there. In this branch, I’ve completely rewritten the code, while the other branch continues to focus on the previous changes or issues

Copy link
Member

Choose a reason for hiding this comment

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

I feel a little lost here. Why can't the calls to dateCalc in the else branch below also be switched to use weekDates?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Apologies for any confusion on my end. I currently have two branches in development: one focused on resolving the current issue, and the other working on issue #651. The second branch is entirely separate, and I need to make some changes to it. As a result, I've converted that branch into a draft and have been focusing solely on the first branch for now. Are we on the same page, or is there still some misunderstanding on my part? This is why I've added the weekDates to this branch and not the other branch .

Copy link
Member

Choose a reason for hiding this comment

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

Oh! Sorry, I meant logical branch of the code, as in the if/else statement that starts at line 399. This PR changes the if portion but does not change the else portion, and I think it should probably change both.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the clarification! I’ve updated both the if and else parts as suggested and pushed the changes. Let me know if anything else is needed! 😅👍

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 +410,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">{weekDates[navIndex].start} - {weekDates[navIndex].end}</span>
</li>
);
}
Expand Down
4 changes: 3 additions & 1 deletion app/assets/javascripts/components/timeline/timeline_handler.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ import {
addWeek, deleteWeek, persistTimeline, setBlockEditable, cancelBlockEditable,
updateBlock, addBlock, deleteBlock, insertBlock, updateTitle, resetTitles, restoreTimeline, deleteAllWeeks
} from '../../actions/timeline_actions';
import { getWeeksArray, getAllWeeksArray, getAvailableTrainingModules, editPermissions } from '../../selectors';
import { getWeeksArray, getAllWeeksArray, getAvailableTrainingModules, editPermissions, getAllWeekDates } from '../../selectors';

// Define TimelineHandler as a functional component using an arrow function
// Move propTypes outside the component definition
Expand Down Expand Up @@ -96,6 +96,7 @@ const TimelineHandler = (props) => {
course={props.course}
weeks={props.weeks}
allWeeks={props.allWeeks}
allWeeksDates={props.allWeeksDates}
week_meetings={weekMeetings}
editableBlockIds={props.editableBlockIds}
reorderable={reorderable}
Expand Down Expand Up @@ -145,6 +146,7 @@ TimelineHandler.propTypes = {
const mapStateToProps = state => ({
weeks: getWeeksArray(state),
allWeeks: getAllWeeksArray(state),
allWeeksDates: getAllWeekDates(state),
loading: state.timeline.loading || state.course.loading,
editableBlockIds: state.timeline.editableBlockIds,
availableTrainingModules: getAvailableTrainingModules(state),
Expand Down
22 changes: 21 additions & 1 deletion app/assets/javascripts/selectors/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +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;
const getCurrentUserFromHtml = state => state.currentUserFromHtml;
Expand Down Expand Up @@ -357,3 +357,23 @@ export const getTicketsById = createSelector(
return tickets.all.reduce((acc, ticket) => ({ ...acc, [ticket.id]: ticket }), {});
}
);

export const getAllWeekDates = createSelector(
[getAllWeeksArray, getCourse],
(weeksArray, course) => {
return weeksArray.map((_, index) => {
const dateCalc = new DateCalculator(
course.timeline_start,
course.timeline_end,
index,
{ zeroIndexed: true }
);
return {
start: dateCalc.start(),
end: dateCalc.end(),
};
});
}
);


Loading