Skip to content
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

Revert "Remove PNG imports" #216

Merged
merged 1 commit into from
Nov 30, 2022
Merged

Conversation

olemartinorg
Copy link
Contributor

@olemartinorg olemartinorg commented Nov 30, 2022

Reverts #215

See also:

This PR broke the Map component when I tested it out in app-frontend-react:

20221130_11h55m53s_grim

@olemartinorg
Copy link
Contributor Author

For reference, this is how the map component is supposed to look (this is using v0.25.1):

20221130_12h02m38s_grim

@TomasEng
Copy link
Contributor

TomasEng commented Nov 30, 2022

Snodig, det fungerer jo i Storybook. Uansett er det ikke bra at vi har disse importene siden de gjør at folk må endre Webpack-konfigurasjonen for å kunne bruke designsystemet, og dermed lar de heller være å ta det i bruk. Hvis det ikke finnes noen bedre løsning, hva om vi lar icon være en egenskap på komponenten som brukeren fyller ut selv? Da vil det kun gå ut over dem som faktisk bruker kartkomponenten. Alternativt kan vi flytte dette ut i et eget repo, som jeg har foreslått tidligere.

@TomasEng
Copy link
Contributor

Just tested this component in Studio, and it works as expected there as well ...

@olemartinorg
Copy link
Contributor Author

Jeg har ikke dykket dypt i hvorfor disse bildefilene ikke ble med i bygget av app-frontend-react etter denne endringen, og selve kartkomponenten er ganske ukjent for meg (den var et eksternt bidrag). Jeg har dessverre ikke ledig tid til en skattejakt for å finne en ny løsning heller... Siden dette nå vil knekke apper i produksjon om vi henter ned siste versjon av designsystemet, og vi er avhengig av å hente ned nye versjoner for å få ut features vi skal levere, foreslår jeg at vi ruller denne endringen tilbake frem til vi finner en løsning for app-frontend-react.

Vi tar gjerne imot en PR i app-frontend-react om dette er noe du brenner for å få til! 🙏

Copy link
Contributor

@TomasEng TomasEng left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tok akkurat en prat om dette med @mijohansen og @mjulstein. Vi prøver å få denne komponenten ut i en egen pakke.

@olemartinorg olemartinorg merged commit 5e2ed0a into main Nov 30, 2022
@olemartinorg olemartinorg deleted the revert-215-fix/remove-png-imports branch November 30, 2022 12:38
@olemartinorg
Copy link
Contributor Author

@TomasEng 👍 Greit for meg, så lenge det ikke også fører til at det blir vanskeligere å ta i bruk designsystemet (om avanserte komponenter lages som egne biblioteker). Håper altså at det ikke fører til at https://designsystem.altinn.studio/ ikke lengre inneholder kartkomponenten fordi den tar i bruk noen bilder, og vi dermed fragmenterer hele designsystemet 1 måned før det dedikerte teamet starter jobben med å ta over vedlikeholdet av designsystemet. Om det er tilfellet, vil jeg heller utsatt denne avgjørelsen slik at de som må leve med den kan ta den.

Og, det kan godt hende det er enkelt å fikse dette i app-frontend-react, jeg har som sagt bare ikke tid til å se på det akkurat nå, og satte ikke pris på at den funksjonaliteten plutselig knakk - det er derfor jeg foreslo å rulle tilbake denne endringen (potensielt midlertidig).

@DanRJ
Copy link
Contributor

DanRJ commented Nov 30, 2022

For dokumentasjonens del:
OED er interessert i å ta i bruk dette prosjektet, slik at man kan gjenbruke relevante komponenter. Det blir en gevinst for oss og forhåpentligvis dere hvis vi får mulighet til å delta.

Vi er derimot ikke interessert i å ta i bruk en pakke som krasjer OED og tvinger oss til å installere ekstra unødvendige pakker (file-loader) eller ikke peker mot visse byggmål / standarder. Dette går på PNG-problematikken men også react-leaflet sin lite fleksible byggmål (React-leaflet maintainer kommentar).

Det ser ut som det finnes ulike måter å komme seg rundt den utfordringen, dog har ikke innsikten eller kompetansen til å vite om det fungerer bra.

Tenker dette er nyttig dokumentasjon for de som tar over vedlikehold av designsystemet. Gjerne legg denne infoen andre steder hvis det er ønskelig @olemartinorg @TomasEng

EDIT: Jeg legger denne kommentaren inn i #214 , fant issuen nå

@olemartinorg
Copy link
Contributor Author

@DanRJ Flott! Jeg kan legge til, om det skulle leses annerledes i kommentaren min over, at jeg er helt enig i at dere ikke skal ta i bruk en pakke som skaper mye friksjon for dere - og denne kartkomponenten var kanskje det.

Jeg mener det er helt greit å knekke bakoverkompabilitet i denne pakken (det har vi gjort før), men da må vi også dokumentere hva man må gjøre for å støtte nyeste versjon. Det skjedde nylig når det ble skilt ut slik at ikonene måtte hentes inn manuelt - og det er helt riktig måte å gjøre det på.

Det jeg ikke likte var å fjerne funksjonalitet en ekstern pakke (app-frontend-react) var avhengig av, i en patch-release, uten noen utredning eller plan for hvordan det skal fikses hos oss. Det fører jo til en blocker i mitt team, som da enten må prioritere brannslukking (for å finne ut av og fikse dette), eller leve med at vi ikke kan oppgradere designsystemet og få inn et eksternt bidrag som står høyt på prioritetslista denne uka (og som i seg selv nå er en blocker for fremdrift).

Jeg legger inn denne saken på boardet i vårt team, slik at noen (forhåpentligvis snart) får sett på hvordan vi kan hente inn v0.25.2 av designsystemet uten at kartkomponenten vår knekker. Om vi finner en god måte å gjøre dette på, ruller vi gjerne frem igjen. Alternativt må enten vi eller OED vedlikeholde en fork i en periode, men ingen av teamene kjenner vel på at de har ansvaret for å bære den kompleksiteten.. 😉

@DanRJ
Copy link
Contributor

DanRJ commented Dec 9, 2022

Godt sammendrag! @olemartinorg Helt enig, OED vil ikke vedlikeholde en fork :) Så det vi har gjort er at vi har modernisert oss og tilpasset oss alle de morsomme pakke-knekkerne, slik at pakken ikke knekker lenger og vi kan endelig nå se om vi kan faktisk ta det i bruk.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants