Translation, extra info from courses.auth, misc bug fixes (language modal breaking app)#305
Translation, extra info from courses.auth, misc bug fixes (language modal breaking app)#305panos21sk wants to merge 23 commits intoacmauth:devfrom
Conversation
neron-png
left a comment
neron-png
left a comment
There was a problem hiding this comment.
Παναγιώτη, πρώτων ευχαριστούμε πάρα πολύ για τη συνεισφορά σου! Προσθέτεις πολύ ωφέλιμες λειτουργίες και διορθώσεις και συγχαρητήρια που με την εμπειρία σου κατάφερες να διαβείς το codebase της εφαρμογής. Είμαι βέβαιος ότι δεν ήταν εύκολο.
Πέρασα όλα τα αρχεία και άφησα μερικά σχόλια που θα ήθελα να τσεκάρεις ή να μου απαντήσεις εξηγόντας μερικά πράγματα.
Αν ήταν να δώσω λίγο γενικότερο feedback όμως, θα ήταν αν μπορείς να ξαναδουλέψεις το pull request, ή κιόλλας να το έσπαγες σε μερικά ξεχωριστά PR ώστε να είναι όλα καθαρά, εύκολα στη κατανόηση και ελέγξιμα. Διότι εδώ μαζί με τις βασικές αλλαγές σου, έχουν περάσει και πολλά ακόμα, όπως indentations σε κάποια αρχεία, διπλότυπος κώδικας, commented out console logs, στυλιστικές και formatting αλλαγές (πχ. κάπου αλλάζεις τα "true" σε {true}, ή το όνομα μιας παραμέτρου title σε name etc.) πράγματα τα οποία δεν αφορούν καθόλου τις βασικές αλλαγές σου και προακλούν περεταίρω μπέρδεμα σχετικά με το λόγο που γίνανε και το τι επηρεάζουν αν επηρεάζουν κάτι. Είναι δύσκολο από τη πλευρά του reviewer να καταλάβει αν κάτι ήταν χαλασμένο και διορθώνεται έτσι, αν έγινε για να γίνει ή αν θα χαλάσει κάτι με κάποια αλλαγή.
Οπότε τσέκαρε ποιά είναι τα βασικά πράγματα που κάνεις στο PR - Μεταφράσεις, εμπλουτισμός του courses.auth, bugfix για το language modal. Κάνε εκ νέου pull το dev, φτιάξε ένα branch για το κάθε ένα, φέρε μόνο τις τελικές αλλαγές σου χωρίς περιττά και κάνε τα ξεχωριστά PR για το καθένα εξηγόντας καθαρά τις αλλαγές σου σε κάθε μέρος που χρειάζεται- ειδικά σε περιπτώσεις που αλλάζει η λογική κάποιου κώδικα, μην είναι μυστήριο γιατί άλλαξε.
There was a problem hiding this comment.
Λίγο μπλεγμένο δεν είναι αυτό; Δεν συνάδει με τα άλλα το course να έχει και localized μορφή. Μήπως να μεταφραζόταν από το api καλύτερα;
There was a problem hiding this comment.
Για το course route δεν μπορούσα να πάρω locale, πάντα ήταν null, και έτσι courseTitle μετεφρασμένο δεν έβγαινε. Αυτό δουλεύει. Υπάρχει θέμα κάπου αλλού; Σε ένα σημείο χρησιμοποίησα αυτό το type.
| gender = personalData.person.gender; | ||
| let subjects = (await neoUniversisGet('students/me/courses?$top=-1')).value; | ||
|
|
||
| let registrations = (await neoUniversisGet("students/me/Registrations?$expand=classes($expand=courseType($expand=locale),courseClass($expand=course($expand=locale),instructors($expand=instructor($select=InstructorSummary))))&$top=-1&$skip=0&$count=false",{lifetime: 600})).value; |
There was a problem hiding this comment.
Κι εδώ θα ήθελα να ήξερα πώς παίζει αυτό και τι σκοπό έχει
There was a problem hiding this comment.
Δεν μεταφράζονται τα ονόματα των μαθημάτων μέσω κλήση στο courses endpoint. Όταν κάνω expand locale πάντα είναι null. Και δεν έχει και άλλο Object με το όνομα του μαθήματος μέσα σε αυτό, για να κάνω expand εκείνο. Οπότε όταν θέλω localized το όνομα του μαθήματος, πρέπει να αντιστοιχήσω το μάθημα απ' την κλήση στο courses με αυτό από το registrations, το οποίο περιέχει τα μεταφρασμένα. Πιο efficient θα ήταν όλα να τα δουλεύουμε με ένα registrations object στο store, αλλά αυτό είναι πολύ δουλεία, αλλά στην μηχανή μου θεωρώντας ότι ένας φοιτητής δεν θα έχει πάνω από 100 μαθήματα έστω, άρα υποθέτω πως δεν θα έχει πραγματική διαφορά στην επίδοση.
2 participants
Fully translated all data obtained from auth's apis that used to be displayed only in greek, besides calendar. Added syllabus, eudoxus fields to course page, and department email along with academic id number in personal info.