-
Notifications
You must be signed in to change notification settings - Fork 0
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
Add registration date to study course list #783
Conversation
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.
Congrats zu deinem ersten Feature 🎉
Sieht alles sehr gut aus. Ich habe dir noch drei Sachen kommentiert, aber das sind nur Details.
@@ -532,6 +532,7 @@ export function buildSubscription( | |||
// AnsweredQuestions: null, | |||
// Messages: null, | |||
// SubscriptionDetails: null, | |||
RegistrationDate: null, |
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.
Hier könntest du auch gleich ein Datum setzen (new Date()
statt null
), dann braucht es nämlich im EventStudentsStateService
das MOCK_REGISTRATION_DATE
gar nicht.
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.
Als ich in events-students-state.service.spec.ts RegistrationDate: New Date()
im expected
hatte anstatt RegistrationDate: MOCK_REGISTRATION_DATE
, hatte ich folgendes Problem:
1) returns all entries per default
EventsStudentsStateService study class entries sort & search
Expected $[0].registrationDate = Date(Wed Feb 05 2025 15:41:25 GMT+0000 (Coordinated Universal Time)) to equal Date(Wed Feb 05 2025 15:41:25 GMT+0000 (Coordinated Universal Time)).
Die Daten wurden nicht richtig verglichen. Denkst du, es könnte das gleiche passieren, wenn ich hier ein Datum setze?
@@ -261,7 +261,8 @@ | |||
"search-placeholder": "Rechercher...", | |||
"list": { | |||
"header": { | |||
"name": "Nom" | |||
"name": "Nom", | |||
"registration-date": "Date d'inscription" |
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.
Vielleicht könntest du dann noch kurz im Ticket Franziska schreiben, dass du es mal so übersetzt hast und sie dir eine andere Übersetzung liefern soll, falls es bei ihnen anders heisst...
}) satisfies StudentEntry, | ||
), | ||
entries: persons.map((person) => { | ||
const subscription = subscriptions.find((s) => s.PersonId === person.Id); |
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.
👌
: 0; | ||
return sortCriteria.ascending ? dateA - dateB : dateB - dateA; | ||
} | ||
|
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.
Wenn du diesen getStudentEntryComparator
jetzt noch ein bisschen mehr optimieren möchtest, könntest du die unterschiedlichen Sortierungslogiken in eigene Methods auslagern, das hilft schon mal den Code leserlicher zu machen, weil es benennt ist:
function getStudentEntryComparator<PrimarySortKey>(
sortCriteria: SortCriteria<PrimarySortKey>,
): (a: StudentEntry, b: StudentEntry) => number {
return (a, b) => {
const key = sortCriteria.primarySortKey;
if (key === "registrationDate") {
return this.compareStudentEntryByDate(a, b);
}
return this.compareStudentEntryByName(a, b);
}
}
Und dann könntest du es noch von der Type-Safety verbessern, in dem du unseren UnreachableError
wirfst, wie hier bei den Open Absences:
Der verwendet den never
Type, was den Vorteil hat, dass falls in der Zukunft ein weiteres Feld hinzukommt, der TypeScript-Compiler motzen würde, wenn hier noch die entsprechende Sortierung fehlt. Ansonsten würde es einfach nach name
sortieren... (weil dies der else-Case ist)
a29824e
to
09d45f2
Compare
#726