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

Filter admin courses #825

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Open

Filter admin courses #825

wants to merge 10 commits into from

Conversation

NIDHI2023
Copy link
Contributor

@NIDHI2023 NIDHI2023 commented Apr 10, 2024

Summary

  • Adds a selection option for courses by semester to admin page
  • Defaults to the current semester in the codebase
    • seems to be SP23, not sure if that's supposed to be updated?

image

  • Displays Archived Courses section only for default semester, only displays filtered courses for rest of the semesters
  • Changed "create course" button theme to match QMI theme more

image

Dependency Changes:

  • run yarn add @mui/material @emotion/react @emotion/styled

Test Plan

-Check if all the desired semesters are available in the dropdown
-Check if the admin page loads the expected default semester, and the archived courses are only visible for this semester
-Check if the correct courses are displayed for each semester
-Check if the "Other" category displays any extraneous courses
-Check if creating a new course while the courses are in their filtered display properly shows up

Checklist

  • [ x] I have updated the documentation accordingly.
  • My PR adds a @ts-ignore

@NIDHI2023 NIDHI2023 requested a review from a team as a code owner April 10, 2024 22:31
Copy link
Contributor

@rgu0114 rgu0114 left a comment

Choose a reason for hiding this comment

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

Functionality looks great, thanks for getting this working so quickly! This will definitely help us navigate the admin page a lot more easily.

Could you also mention in the PR description the dependency changes you made? e.g. the commands you may have run to change what's in the yarn.lock file. Some changes like using sass instead of node-sass were already merged into master, so I just want to double check that the dependencies are correct. Also, be sure to resolve merge conflicts and the pipeline failing issues (lmk if you need help with either of those, might be confusing).

Copy link
Contributor

@rgu0114 rgu0114 left a comment

Choose a reason for hiding this comment

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

I'll also add more code comments sometime tomorrow + we'll loop in the designers to see if they have any suggestions.

@dti-github-bot
Copy link
Member

[diff-counting] Significant lines: 62.

@rgu0114
Copy link
Contributor

rgu0114 commented Apr 15, 2024

Could you try making the filter selection background white so that it doesn't blend in with the grayish page base color too much? There should be an option to do that. A bit wider by default as well.

Screen Shot 2024-04-15 at 6 47 15 PM

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.

None yet

3 participants