-
Notifications
You must be signed in to change notification settings - Fork 63
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: implement Course Statuses feature #444
base: main
Are you sure you want to change the base?
Conversation
Your pull request title did not conform to conventional commits standards. Our upcoming automated release pipeline will automatically determine
|
1 similar comment
Your pull request title did not conform to conventional commits standards. Our upcoming automated release pipeline will automatically determine
|
@@ -115,6 +115,7 @@ const splashText: string[] = [ | |||
`It's ${new Date().toLocaleString('en-US', { month: 'long', day: 'numeric' })} and OU still sucks`, | |||
'As seen on TV! ', | |||
"Should you major in Compsci? well, here's a better question. do you wanna have a bad time?", | |||
"https://i.redd.it/4y3fc2bzva1e1.gif", |
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.
This is not related to this PR in addition that we shouldn't include random links. Please remove.
import { resolve } from "path"; | ||
|
||
|
||
export default async function checkCourseStatusChanges() { |
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.
Needs jsdoc.
@@ -0,0 +1,67 @@ | |||
import { UserScheduleStore } from "src/shared/storage/UserScheduleStore"; |
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.
Use path alias.
const scrapedStatus = scraper.getStatus(row)[0]; | ||
newStatus = scrapedStatus as StatusType; |
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.
Ideally we should have some validation here.
const scrapedStatus = scraper.getStatus(row)[0]; | ||
newStatus = scrapedStatus as StatusType; | ||
} | ||
console.log('Unique ID:', uniqueId); |
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.
This should have some further context if needed for logging. Otherwise this is just verbose and should be removed.
import SettingsIcon from '~icons/material-symbols/settings'; | ||
import checkCourseStatusChanges from 'src/pages/background/handler/courseStatusChange'; |
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.
Use path alias.
useEffect(() => { | ||
console.log(enableCourseStatusChips, "settings"); | ||
},[enableCourseStatusChips]) |
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.
debug console.log. Should be removed later on.
@@ -138,7 +141,7 @@ export default function Settings(): JSX.Element { | |||
// Listen for changes in the settings | |||
const l1 = OptionsStore.listen('enableCourseStatusChips', async ({ newValue }) => { | |||
setEnableCourseStatusChips(newValue); | |||
// console.log('enableCourseStatusChips', newValue); | |||
console.log('enableCourseStatusChips', newValue); |
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.
debug console.log. Should be removed later on.
@@ -211,7 +211,7 @@ export class CourseCatalogScraper { | |||
*/ | |||
getInstructionMode(row: HTMLTableRowElement): InstructionMode { | |||
const text = (row.querySelector(TableDataSelector.INSTRUCTION_MODE)?.textContent || '').toLowerCase(); | |||
|
|||
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.
Random indentation.
{/* <button className='inline-block h-4 w-4 bg-transparent p-0 btn'> | ||
<button | ||
className='inline-block h-4 w-4 bg-transparent p-0 btn' | ||
onClick={async () => await checkCourseStatusChanges()} |
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.
nit. I would extract this into a dedicated function.
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.
how is this any different from onClick={checkCourseStatusChanges}
?
Also please make sure that all checks are passing. |
okay so i've been working on this PR for the past 2 days and i think i've almost got it. i'm currently having an issue with typescript being mad at me when i'm updating the usersSchedule with the updated course status
the issue is in: courseStatusChange.ts
the specific error is: where i'm getting the error: Type '{ courses: Course[]; id?: string | undefined; name?: string | undefined; hours?: number | undefined; updatedAt?: number | undefined; }' is not assignable to type 'Serialized'.
Types of property 'id' are incompatible.
Type 'string | undefined' is not assignable to type 'string'.
Type 'undefined' is not assignable to type 'string'.ts(2322)
and has to do when I'm updating the schedule.
i'm get why it's concerned about it potentially being undefined, but there is no possible way it can be undefined since the currentSchedule.map returns a course with the course type.
lmk if you have any questions, i've bunch of random things, but nothing seems to work.
also, FYI, ik the code is super messy rn, I'm just trying to get everything to work and then I'll clean it up
This change is