-
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
Endre på areal for hver kvadrant #1
Conversation
@@ -16,7 +16,8 @@ | |||
"react": "^18.3.1", | |||
"react-dom": "^18.3.1", | |||
"react-router-dom": "^6.26.0", | |||
"react-tooltip": "^5.28.0" | |||
"react-tooltip": "^5.28.0", | |||
"zustand": "^5.0.1" |
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.
Ikke interesert i å introdusere zustand her. Zustand brukes som en "ekstern store" når en har data som skal endres. All data her er statisk, dette introduserer kun ekstra kompleksitet. Om du kun skal endre på areal for kvadranter så hold deg til endringer som kun har med dette å gjøre.
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.
Tanken med Zustand her er å holde styr på hvilken blip som er highlighted/valgt, ikke endre på noe data sådan. Den erstatter const [currentBlip, setCurrentBlip] = useState()
og 3 nivåer med prop-drilling og utvider dette til å kunne sette en item man hovrer over for å highlighte tilhørende blip/<li
> rundt radaren. (Hentet denne ideen fra fagradaren du viste frem forrige arbeidsmøte)
Man kan bruke Zustand til mer avanserte formål som du beskriver, men det funker ypperlig som et pent lite store for å slippe å skrive 30-40 linjer boilerplate for å lage en React Context/Provider. 😮💨
Det kan kanskje være litt misvisende at denne endringen kommer med i en PR som heter "Endre på areal for hver kvadrant", men jeg kan oppdatere beskrivelsen til å inkludere dette?
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.
Jeg er enig i at zustand er bedre enn React Context/Provider, jeg bare syns det introduserer mer kompleksitet her hvor en useState hadde holdt lenge...
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.
Vanskelig å følge hvilke logiske endringer du har gjort når du endrer på masse formatering og ting som ikke er rellevant. Jeg er også i prosessen med å refaktorere litt på radar/index.tsx
, så kanskje vent til etter det er ferdig?
45c245f
to
d9564a6
Compare
Litt vanskelig å "unformate" kode-endringene nå, kanskje det kunne vært en tanke å installere f.ex Biome for å få inn en gitt standard for formattering, når det kommer utviklere inn med format-on-save som defaulter til Prettier defaults når ingenting er satt (👋 )? Biome er ganske smooth, både lint og format i en. Hovedchunken med endringer som gjelder for det innerste nivået finnes mellom linje 141:259 i capra-fagradar/src/radar/index.tsx, som fordeler blipsa i depth === 1 utover 3 forskjellige rader. Har ikke stress-testa denne med 100 blips, men håper vi ikke kommer til et punkt der vi har så mange 🙃 Selve areal-endringen isolert sett skjer nok med linje 141 og 332. Oppdatere areal alena gav ingen mening når den innerste sirkelen ble større, og blips ble sittende fast i den trange rekken den stod. Derfor ble omfordeling av blips med i PR. (Med en god del hjelp av ChatGPT, skal ikke prøve å forklare alt som skjer der.) Fiksa konfliktene med index fila! |
Enig i at detg burde være en backlog oppgave å sette opp Biome. Likevel foreslår jeg at du skrur av "format on save" siden det ikke er satt opp noe i prosjektet og ser på om du klarer å fikse formateringen manuelt slik at det er mulig å reviewe koden |
Veldig fint med mer oversikt! Vil bare legge til at jeg synes det ytre nivået der ble Iitt tynn, men ellers fint 😊 |
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.
You know what, fiks merge konflikten, så merger vi det bare. Vi kan alltids endre ting videre.
d6537c3
to
bf85365
Compare
Kort oppsummert:
Fikk ChatGPT til å hjelpe meg med å skrive om quadrant komponenten til å kunne:
Før:
Etter: