AtB-AS / webshop

AtB Webshop / AtB Nettbutikk
https://nettbutikk.atb.no
European Union Public License 1.2
0 stars 1 forks source link

feat: changes to use @atb-as/generate-assets #426

Closed mikaelbr closed 2 years ago

mikaelbr commented 2 years ago

Skriver om til å bruke eksterne assets som blir hentet ned via pakke @atb-as/generate-assets. Som en del av det arbeidet legger den også opp til en mer fleksibel måte å håndtere light/dark-images hvor det er støtte for egne ikoner og egne illustrasjoner på forskjellige modes.

Inkluderer:

TODO:


Er avhengig av release av https://github.com/AtB-AS/design-system/pull/11

github-actions[bot] commented 2 years ago

Visit the preview URL for this PR (updated for commit aa81f67):

https://atb-webshop-staging--pr426-mikael-use-generate-69up7swg.web.app

(expires Sun, 28 Nov 2021 13:27:56 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

mikaelbr commented 2 years ago

@tormoseng Jeg la til deg til review her jeg. Det er en ganske stor endring på en måte, som egentlig ikke skal synes. Det er lagt til rette for å bedre kontrollere ikoner på dark mode/light mode. Som f.eks nå også da fikser feil farge på transport-ikoner på darkmode i onboarding (om du har sett feilen der). Hadde vært fint å klikka litt igjennom for å se om dette introduserer noe åpenbare feil som jeg burde fikse.

mikaelbr commented 2 years ago

For de som måtte være interessert: Bumper byggescripts node versjon til 16. Trenger ikke være 12 (og er avhengig av det for nyere JS syntaks i en avhengighet)

mikaelbr commented 2 years ago

Jeg har klikket igjennom for både NFK og ATB i darkmode og lightmode, men kan være det er noe med f.eks responsivitet jeg har missa.

mikaelbr commented 2 years ago

Det er en ulempe med bruk av custom elements her. Safari har ikke støtte for "customized built in elements". Som i praksis gjør at vi ikke kan extende img som jeg egentlig ønsket å gjøre. Det vil si at det må være en autonomous element, og da at bildet må være et barn. Med andre ord, vi går fra <img> til <div><img/></div>. Det har konsekvenser for layout og det er vanskelig å behandle det på samme måte som vi har gjort før. Det kan derfor være enkelte plasser hvor man ønsker å bruke et bilde men får en div. Det har f.eks konsekvenser for høyden på elementer. Greier ikke å tenke ut en umiddelbar løsning på det nå uten mye sofistikert ekstra-javascript for å i større grad håndtere størrelser på div osv. Men det er og en slippery slope her.

tormoseng commented 2 years ago

@mikaelbr Ser ut til at vi mangler Visa/MC-ikon i staging nå.

Her for oppsummeringssiden:

Screenshot 2021-11-29 at 13 31 33

Her for lagrede betalingskort:

Screenshot 2021-11-29 at 13 32 12

mikaelbr commented 2 years ago

Ajaj. Jeg ser på det nå.

mikaelbr commented 2 years ago

@tormoseng https://github.com/AtB-AS/webshop/pull/433

tormoseng commented 2 years ago

Ellers kjørte e2e-testene fint gjennom. Men mtp ikonene over så sjekker kun e2e-testene at det ligger et img-element der med sti som jeg har angitt. Burde se nærmere på mer visuell testing. Vet Cypress har noe plugins (+ eksterne biblioteker) som kan benyttes. På Todo-lista.

mikaelbr commented 2 years ago

Ja, det kan jo være verdt å sjekke ut om det gir noe verdi. Ofte blir sånne visuelle diff tester fulle av false negatives, men det kommer sikkert litt an på nivået det legges på og.

tormoseng commented 2 years ago

Ja, det kan jo være verdt å sjekke ut om det gir noe verdi. Ofte blir sånne visuelle diff tester fulle av false negatives, men det kommer sikkert litt an på nivået det legges på og.

Mm. Må være ganske spesifikke tester. For eksempel "Mine billetter" siden vil jo aldri fungere da gyldighetstid endres rimelig konstant.