-
Notifications
You must be signed in to change notification settings - Fork 11
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
Refactor semesters to plans #858
base: main
Are you sure you want to change the base?
Conversation
[diff-counting] Significant lines: 238. |
Visit the preview URL for this PR (updated for commit bbf321e): https://cornelldti-courseplan-dev--pr858-refactor-sems-s7voxvuv.web.app (expires Tue, 21 Nov 2023 23:37:53 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: 6d4e0437c4559ed895272bbd63991394f1e0e933 |
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.
Looking good, just some clean up items, I would do a command f for semesters just in case you missed something, did you ever end up fixing that bug btw?
@@ -107,7 +107,14 @@ export default defineComponent({ | |||
}, | |||
computed: { | |||
semesters(): readonly FirestoreSemester[] { | |||
return store.state.semesters; | |||
console.log(store.state.plans); |
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.
get rid of console log
src/user-data.d.ts
Outdated
}; | ||
type FirestoreOverriddenFulfillmentChoices = { | ||
readonly [courseUniqueId: string]: FirestoreCourseOptInOptOutChoices; | ||
}; | ||
|
||
type FirestoreUserData = { | ||
readonly name: FirestoreUserName; | ||
readonly semesters: FirestoreSemester[]; | ||
// readonly semesters: readonlyFirestoreSemester[]; |
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.
delete
src/store.ts
Outdated
updateDoc(doc(fb.semestersCollection, simplifiedUser.email), { | ||
plans: [{ semesters }], // TODO: andxu282 update later | ||
plans: [plan], // TODO: andxu282 update later |
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.
can u get rid of comment 🥲
src/store.ts
Outdated
@@ -300,7 +337,7 @@ export const initializeFirestoreListeners = (onLoad: () => void): (() => void) = | |||
store.commit('setSemesters', [newSemester]); | |||
setDoc(doc(fb.semestersCollection, simplifiedUser.email), { | |||
orderByNewest: true, | |||
plans: [{ semesters: [newSemester] }], // TODO: andxu282 update later | |||
plans: [{ name: 'Plan 1', semesters: [newSemester] }], // TODO: andxu282 update later |
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.
remove comment : )
src/store.ts
Outdated
@@ -76,7 +78,7 @@ const store: TypedVuexStore = new TypedVuexStore({ | |||
tookSwim: 'no', | |||
}, | |||
orderByNewest: true, | |||
semesters: [], | |||
// semesters: [], |
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.
delete
src/store.ts
Outdated
@@ -36,7 +36,7 @@ export type VuexStoreState = { | |||
currentFirebaseUser: SimplifiedFirebaseUser; | |||
userName: FirestoreUserName; | |||
onboardingData: AppOnboardingData; | |||
semesters: readonly FirestoreSemester[]; | |||
// semesters: readonly FirestoreSemester[]; |
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.
delete
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.
I think it would probably be easier and cleaner if you modified the semesters
getter to return plans[currentPlan].semesters
. That way, accesses to store.state.semesters
wouldn't have to be changed throughout the codebase.
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.
Instead of passing the current plan into functions like addCourseToSemester
and deleteCourseFromSemesters
, can we import the vuex store and access the current plan directly inside of these functions?
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! Can you delete the spurious console statements?
@@ -455,15 +470,28 @@ export default defineComponent({ | |||
this.openConfirmationModal(`Added ${course.code} to ${this.season} ${this.year}`); | |||
}, | |||
deleteCourseWithoutModal(uniqueID: number) { | |||
deleteCourseFromSemester(this.year, this.season, uniqueID, this.$gtag); | |||
deleteCourseFromSemester( | |||
store.state.currentPlan, |
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.
Why do we need to pass the current plan as an argument?
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.
I guess there's no case where you aren't deleting from the current plan currently but it was just the way I wrote the firestore functions to modify the plan that is passed in.
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.
Summary
Remove all uses of semesters in Vuex store and in Firebase data
10/22/23 Changes
Remaining TODOs:
Test Plan