-
Notifications
You must be signed in to change notification settings - Fork 2
Ax/scrum 101 Finish Timetable Home Page #50
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
Conversation
…UTSC-CSCC01-Software-Engineering-I/term-group-project-c01w25-project-course-matrix into ax/scrum-101-timetable-frontend
kevin-lann
left a comment
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.
Small suggestion regarding using default button color for consistency, but other than looks perfect, really close to mockup.
course-matrix/frontend/src/pages/Home/TimetableCompareButton.tsx
Outdated
Show resolved
Hide resolved
course-matrix/frontend/src/pages/Home/TimetableCreateNewButton.tsx
Outdated
Show resolved
Hide resolved
Co-authored-by: Kevin Lan <[email protected]>
….tsx Co-authored-by: Kevin Lan <[email protected]>
kevin-lann
left a comment
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 good. Nice job matching the mockup
…UTSC-CSCC01-Software-Engineering-I/term-group-project-c01w25-project-course-matrix into ax/scrum-101-timetable-frontend
kevin-lann
left a comment
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 good overall, just a few things (Not high priority but can consider for enhancements in future):
-
UI: Edit timetable text appears on two lines instead of 1
-
One improvement that could be made (for future) is to not reload the page when deleting a timetable. A more standard way to do this is to have the getTimetablesQuery erfetch after deletion. Redux provides a refetch callback fn that could be used like this in Home.tsx:
const { data, isLoading, refetch } = useGetTimetablesQuery() as {
data: Timetable[];
isLoading: boolean;
};Then this refetch fn can be passed down using props (or context API) to TimetableCardKebabMenu and be used in the handleDelete function:
const handleDelete = async () => {
try {
await deleteTimetable(timetableId);
refetch() // replaced window.location.reload();
} catch (error) {
console.error("Failed to delete timetable:", error);
}
};This will get timetables again and will rerender the Home component instead of reloading the page itself.
…UTSC-CSCC01-Software-Engineering-I/term-group-project-c01w25-project-course-matrix into ax/scrum-101-timetable-frontend
…UTSC-CSCC01-Software-Engineering-I/term-group-project-c01w25-project-course-matrix into ax/scrum-101-timetable-frontend
Fixed. Thanks. |
|
Here is an issue that I found while testing: When I have multiple timetables, and I refresh on any one of them, after refresh it will be always refresh to the timetable which was clicked first, despite switching to a different one from the dropdown menu. |
Thanks, fixed. |
thomasyzy7
left a comment
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.
LTGM, tested all I think currently think of.
MasahisaSekita
left a comment
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.
seems functional to me, bugs are gone and its working.



Description
SCRUM-101
Figure 1: Home Page

Figure 2: The Calendar on the Timetable Builder Page

Type of change
Checklist: