Skip to content

Conversation

@Austin-X
Copy link
Contributor

@Austin-X Austin-X commented Mar 2, 2025

Description

SCRUM-101

  • Implemented The Home Page which lists out all of the user's timetables (see Figure 1 below)
  • Implemented the Calendar on the Timetable Builder page (See Figure 2 below)
    • Adding/removing entries and adding/removing restrictions functionality have not bee implemented yet. Also, the "Save Timetable" button functionality have not been implemented yet. They will be implemented in a future sprint.

Figure 1: Home Page
image

Figure 2: The Calendar on the Timetable Builder Page
image

Type of change

  • New feature (non-breaking change which adds functionality)
  • Bug fix (non-breaking change which fixes an issue)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

Checklist:

  • My code follows the style guidelines of this project
  • I have commented my code, particularly in hard-to-understand areas
  • My changes generate no new warnings

@Austin-X Austin-X changed the base branch from main to develop March 2, 2025 07:03
@Austin-X Austin-X marked this pull request as ready for review March 3, 2025 02:09
Copy link
Contributor

@kevin-lann kevin-lann left a 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.

@kevin-lann kevin-lann self-requested a review March 4, 2025 03:12
Copy link
Contributor

@kevin-lann kevin-lann left a 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

@thomasyzy7
Copy link
Contributor

LTGM,

Future things to consider:
Something I did notice so far is when clicking the semester dropdown menu, the scrollbar to the right disappears:
image
image
(second image is when dropdown is clicked, it is barely visible, I snipped the image the moment i click the dropdown).

@thomasyzy7
Copy link
Contributor

Furthermore, the month view looks extremely cursed right now:

image

Copy link
Contributor

@kevin-lann kevin-lann left a 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):

  1. UI: Edit timetable text appears on two lines instead of 1

  2. 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.

@Austin-X
Copy link
Contributor Author

Austin-X commented Mar 7, 2025

Looks good overall, just a few things (Not high priority but can consider for enhancements in future):

  1. UI: Edit timetable text appears on two lines instead of 1
  2. 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.

Fixed. Thanks.

@thomasyzy7
Copy link
Contributor

thomasyzy7 commented Mar 7, 2025

@Austin-X

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.

@Austin-X
Copy link
Contributor Author

Austin-X commented Mar 8, 2025

@Austin-X

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.

@Austin-X Austin-X changed the title Ax/scrum 101 Timetable Frontend Ax/scrum 101 Finish Timetable Home Page Mar 8, 2025
Copy link
Contributor

@thomasyzy7 thomasyzy7 left a 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.

Copy link
Contributor

@MasahisaSekita MasahisaSekita left a 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.

@Austin-X Austin-X merged commit f57f4de into develop Mar 8, 2025
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants