UX: - [ ] Dere bør se på om det er mulig å implementere litt tykkere skrift ved hover i ResultsCards med noe annet enn skygge, det skyldes av at nettlesere bandler skyggen ulikt og kan medføre ulesbar tekst i noen. Noen alternativ er grade axis (font-variation-settings) og -webkit-text-stroke. - [ ] Hadde vært kult om dere hadde fått til å "gray out" de valgalternativene som leder til et tomt resultat. f.eks. difficulty H leder til tomt resultat - [ ] det eneste er at tittelen er litt liten på mobil. - [ ] jeg ville muligens gitt clear filters en "farlig" farge, vertfall på hover. - [ ] 'Når jeg åpner et element fra lenger ned i lista, beholdes scrollposisjonen, så jeg lander midt på siden i stedet for ved overskriften. - [ ] På mobil tar hvert kort nesten en hel skjerm, så det blir mye scrolling for å se 20 resultater, - [ ] det mørke temaet følger ikke operativsystemets innstilling ved første besøk. og Dark mode-toggle leser bare localStorage i App.tsx og sjekker aldri prefers-color-scheme, så en bruker med mørkt systemtema likevel får lyst tema ved første besøk. UI / farger - [ ] ResultsTable.tsx har en ordentlig gjennomtenkt løsning for tabell på desktop og kort på mobil, med god kommentering av hvorfor. Dark mode er derimot ikke helt gjennomført: i variables.css har tokens som –page-bg og –surface egne mørke verdier, men –secondary-color, –background-color-element og –text-color dark har bare en definisjon og ingen override i [data-theme=‘dark’]-blokken. Element-detaljsiden bruker nettopp disse tre (bg-(–secondary-color) i ElementDetailPage.tsx), så den siden blir stående lys og utenfor stil når resten av appen går i dark mode. Det er litt inkonsistens i et ellers ryddig fargesystem. - [ ] Til slutt strekkes bildene på detaljsiden langt over sin egentlige størrelse, og derfor blir de uskarpe. Med en maksbredde på bildet, eller bilde og info i to kolonner, slik som i mockupen deres, ville det sett skarpere ut og utnyttet bred skjerm bedre. (samme fra annen) Elementtegningene er 400 px brede, men vises opptil 1260 px brede på elementsiden og blir tydelig uskarpe. - [ ] Dere kan vurdere å bruke samme font på både tittler og tekst med tanke på tilgjengelighet og ensformighet. - [ ] Mørk modus bør også være mer konsekvent - [ ] . Stylingen blander også Tailwind, shadcn, egne CSS-variabler og rester av Vite-malen. Dette skaper problemer, og kan også forklare litt av feilene deres med mørkt tema. Teknsike valg: - [ ] Vite proxy er brukt på en god måte for å unngå problemer med CORS internt på serverern, men koden som brukes i frontend for å vise fram bilder bruker ikke den proxien og kan være grunnen til at bildene ikke vistes da jeg testet siden. - [ ] I tilegg ville jeg lagt til milløvariabler til konfiguratsjonen slik at frontend kan ta bruk av vm-ens backend når dere arbeider med pur frontend lokalt. - [ ] Eneste å kommentere på er bundlingen: npm run build gir en JS-fil på 721,75 kB (228 kB gzip), over Vites 500 kB-grense, og det er ingen lazy() eller route-splitting jeg ser i App.tsx selv om appen har seks ruter. Det stemmer for øvrig ikke med tallet README og SUSTAINABILITY.md oppgir (548 kB), så dokumentasjonen har ikke blitt oppdatert etter siste endringer. - [ ] **Ville vurdert å skrudd av den apollo sandbox greia som ligger under /graphql. (NB dette må vi vurdere nøye sammen i gruppen)** - [ ] Noen valg bør likevel vurderes. Cachen er satt opp for å gjenbruke søk, men da jeg gikk tilbake fra et element, ble første side hentet på nytt og lista falt fra 40 til 20 rader, stikk i strid med det SUSTAINABILITY.md beskriver. Det er verdt å sjekke om «load more»-spørringen henter andre felt enn første side, slik at Apollo regner cachen som ufullstendig. - [ ] Innloggingen er dessuten delt i to: en lokal isLoggedIn-bryter som styrer den beskyttede ruten, og en ekte sesjon med tokens. Ruten burde vært styrt av sesjonen. Refresh-tokenet lagres i localStorage, som er mer utsatt ved XSS enn en HttpOnly-cookie, og introspeksjon er på i produksjon. - [ ] I ResultsTable.tsx blir både mobil-kortene og desktop tabellen rendret samtidig til DOM-en gitt md:hidden/max-md:hidden. Dere kan vurdere å endre det ved å f.eks. bruke ResizeObserver API, Tailwind-Aware hook eller en debounced/thottled resize event listener. (#201) - [ ] Jeg ville vurdert å lagt til en debounce på filtrering, spesielt multi-selektbar vanskelighetsgrad, siden det hender ofte at man vil selektere flere på en gang og da er det unødvendig med et nytt kall for hvert klikk. (#202) Kodekvalitet - [ ] Testfilen på 1375 linjer er også lang - [ ] src/assets inneholder fortsatt Vite- og React-malfiler som ikke brukes, og ryddes bort. - [ ] Noen komponenter bruker camel case og andre Pascal case, bruk Pascal case entydig. - [ ] Det er også noen components som egentlig kunne vært slått sammen, for eksempel GymnastRow som hvis jeg ikke ser feil, kun brukes i Gymnast List, hvor begge to er ganske små. - [ ] Dokumentasjonen er svært omfattende (README er på 45 kB og API.md på 80 kB), så en kortere oversikt med lenker videre ville gjort det lettere å finne fram. - [ ] Et forbedringspunkt er at noen komponenter har svært lange Tailwind-klasser og ganske detaljerte CSS-overstyringer. Dette gjør stylingen vanskeligere å lese og endre. Flere felles komponentvarianter kunne gjort dette mer oversiktlig. Skalerbarhet: - [ ] Et problem er når man går inn på en øvelse og så tilbake. Da hentes listen på nytt, og man mister alle dem man har hentet inn tidligere. - [ ] Det eneste jeg ville sett på, er at hvert element i svaret gjentar hele navnet på elementgruppen og apparatet, selv om strukturen allerede hentes i en egen spørring. Det ville holdt å sende id-en. Den manglende gjenbruken av cache ved tilbakenavigering er nevnt under tekniske valg. Lagring av brukerdata - Hovedsaklig ting vi har fikset + å klage over ting vi ikke har implementert, fair Bærekraft - [ ] Bildene utgjør 7 MB totalt, så en vurdering av størrelser per skjermbredde hadde passet godt her. - [ ] JS-bunten er fortsatt 548 kB (171 kB gzip), og dere har ingen kodesplitting med React.lazy per rute, så den kan med fordel bli mindre. og . Det største sustainability-hullet er bundlestørrelsen på 721 kB uten kodesplitting, som direkte motsier intensjonen i SUSTAINABILITY.md om å begrense dataoverføring. og Den største posten er JavaScript. Bundlen i produksjon er rundt 700 kB ukomprimert og 220 kB gzippet, mer enn tallene i SUSTAINABILITY.md, og den er ikke delt opp per rute. - [ ] Siden deres er ganske stor, ca 220kB, inkludert med at jeg teller 7 forskjellig fonter som blir lastet inn fra google. (ops) - [ ] nevne at vi bruker .wepp som bildeformat, bra for bildestørrelse - [ ] Fontene hentes fra Google Fonts med ni vekter fordelt på to familier, og filene med hash i navnet sendes uten cache-header. Kodesplitting, færre fontvekter og lang cache på assets ville gitt merkbar gevinst. - [ ] Lucid icons kan ikke "Tree shake"-es som standard, men det er mulig å bruke Babel, custom rollup plugin eller direct imports. Brotli-komprimering kan også hjelpe. (#203) Tilgjengelighet: - [ ] Dere skriver ærlig at skjermleser ikke er testet, og det er det eneste som mangler for å få en helhetlig vurdering. Dere kan rpøve NVDA eller VoiceOver og dokumenter funnene. - [ ] README hevder sorteringen følger “ARIA Authoring Practices” med aria-sort på kolonneoverskriftene, men SortMenu.tsx bygger en radiogruppe i en dropdown-meny, og jeg fant ingen aria-sort noe sted i src. Enten er README skrevet for et annet design enn det som endte opp i koden, eller det ble aldri lagt til. - [ ] Det kunne derimot vært litt mer effekt på knappene når de er hovret over av musen, det var litt vanskelig å se nå - [ ] Bildene kunne kanskje hatt en litt tydligere alt text, nå er ikke jeg turner, men tror jeg hadde slitt litt med "Illustration for 1.00", dere har allerede veldig gode beskrivelser under tittelen, de ville jeg heller ha brukt. - [ ] Fokus flyttes ikke til innholdet ved navigasjon, og etter «Load more» havnet fokus på body fordi knappen deaktiveres mens den laster. Da jeg valgte sortering med tastaturet, ble menyen stående åpen, så det er uklart om valget ble tatt. ki bruk: - [ ] KI kunne med fordel vært brukt mer til å generere tester for kantcaser og til å gjennomgå tilgjengelighet. og Det dere kan bruke KI til i fremtiden er å få dem til å sjekke pull requestene deres, slik at den kunne fanget at det ligger igjen rester fra Vite-malen, og at titlene blir usunlig i mørkt tema. - [ ] Men det er viktigere at dere sørger for at dokumentasjonen holdes oppdatert etter hvert som koden endres. og Å la KI sammenligne dokumentasjon mot faktisk kode og build-output ved hver PR kunne fanget driften før den samlet seg opp. og En siste KI-runde som sammenligner README og dokumentasjonen med appen på VM-en, kjører Prettier på hele kodebasen og analyserer bundlen, ville vært en enkel gevinst. - [ ] Dokumentasjonen deres er også veldig lang, så dere kan bruke KI til å korte den ned. - [ ] KI har bidratt til ganske store deler av prosjektet, blant annet backend, sesjonshåndtering, frontend og tester. Derfor er det viktig at dere også vurderer hvordan delene fungerer sammen. - [ ] **Det dere gjerne kunne hatt med er hva dere lærte av å bruke KI, fordi nå er det mer en logg enn en refleksjon.** - [ ] Videre kunne dere brukt KI til å sammenligne dokumentasjonen med implementasjonen og foreslå tester av hele brukerflyter. Dette kunne vært nyttig for å oppdage både utdatert informasjon og manglende koblinger mellom frontend og backend. Allerede gjort / planlagt - [ ] ElementsPage.tsx på 422 linjer gjør mye og bør kanskje deles opp, for eksempel med en egen hook som useElementSearch. og fra annen kommentar: Noen komponenter bør deles opp i mindre biter, slik som ElementsPage, der tilstands- søkelogikk og hooks bør adskilles - [ ] Jeg ville nok organisert test filene i en egen mappe, nå ligger alt sammen inne i samme folder, som det de tester. Det kan bli litt vanskelig å navigere i. - [ ] Components kunne også vært litt bedre strukturert, det tok en liten stund før jeg fant fram til hva soim var de faktiske sidene deres. Om dere hadde hatt noe form for "pages" eller noe lignende også components med knapp og generelle ting, så hadde det kanskje vært lettere å navigere. - [ ] paginering ui - [ ] Noen konkrete målinger av overført datamengde ved første besøk og ved gjenbesøk ville gjort det enda tydeligere hvilken effekt løsningene har. Vet ikke helt hva dette betyr finn ut - [ ] Tror nokk systemet deres er ganske bra som det er, må si at det er veldig stort og ganske vanskelig å komme seg rundt da... x2 og nå klarte jeg oppriktig talt ikke finne fram til hvor dere gjør søkene i databasen, - [ ] to punkter til fra peer reviews under kodekvalitet, tekniske valg, design og utforming og funksjonalitet som jeg med et uhell slettet. - [ ] I localstoragen min var det lagret nøyaktig 3 ting, catapphistory (tom liste), catappindex (tallet -1) og theme (light) (Som forsåvidt også kunne hentes fra browser, jeg har dark mode på, men siden er i light mode) Jeg vet ikke helt hvordan jeg har fått dette i localstoragen fra denne siden, men jeg er i det minste rellativt sikker på at den nåværende siden ikke har så utrolig mye med katter å gjøre... Jeg tror ikke dere har så mye lagret i databasen heller, ettersom jeg ikke fant noe felt som jeg kunne sende ting i (hvis ikke dere lagrer alle søkene mine da, men det er det ikke veldig myte som tyder på). - [ ] Men dere har per nå veldig mye kode, som ikke gjør så alt for mye totalt sett. Det er ganske beskrivende på prosjekt hvor det er brukt ganske mye KI for utvikling.