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

fix: redcross and greencheck now uses material icons font #4491

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

olejorgenbakken
Copy link
Contributor

ISSUES CLOSED: #3793, #3939

Denne er litt tricky, og ikke 1 til 1 med Figma. Det var litt tricky success og error bruker den vanlige fonten for å vise ikonene.

I kort betyr det at det er maskering i midten av ikonet som gjør at det er enkelt å sette fargen til samme farge som tekst. I de fargede ikonene er det derimot tenkt at "maskeringen" er erstattet med svart farge i både light og dark mode. Det hadde gjort komponentene veldig komplekse i forhold til hva den er om vi respekterer samme prinsipp i de fargede variantene også.

Det kan godt hende at den kompleksiteten er verdt det, eller at det finnes en enklere måte å gjøre det på som jeg ikke vet om. Gjerne del det i tråden her eller noe altså 😸

🎯 Sjekkliste

  • Nye features er dokumentert (sjekk Contributing om du er usikker på hva som trengs av dokumentasjon)
  • Testet responsivitet og UU (tastaturnavigasjon, skjermleser)
  • pnpm build og pnpm ci:test gir ingen feil (pnpm ci:test gir noen feil som ikke er knytta til endringer i denne PR-en såvidt jeg kan se, vet ikke om noen har noe info om hva det kommer av?)

@olejorgenbakken
Copy link
Contributor Author

Her er eksempler på hvordan ikonene ser ut i denne PR-en:
image

Her er de i Figma:
image

Tok med noen av ikonene ved siden av for å vise at de ser litt utilpass ut fordi de blir så lyse. Det kan man jo fikse på mange måter, men man må vel først bestemme løsningen man vil gå for i kommentaren over tror jeg 🤷

@fremtind-bot
Copy link
Collaborator

fremtind-bot commented Jan 23, 2025

Forhåndsvisning: https://jokul.fremtind.no/preview/material-icons-check-cross/
🔍 Commit: 2924f92

Forhåndsvisningen blir tilgjengelig innen et par minutter. Den fjernes automatisk når pull requesten lukkes.

fremtind-bot added a commit that referenced this pull request Jan 23, 2025
@olejorgenbakken olejorgenbakken self-assigned this Jan 23, 2025
@piofinn
Copy link
Contributor

piofinn commented Jan 23, 2025

Jeg tror vi hadde landet litt på at den ekstra kompleksiteten er verdt det inntil videre. Med et mer robust fargesystem (som jo er ganske rett rundt hjørnet) kan vi velge farger for de ikonene, slik du nå har implementert dem, som sørger for bra kontrast mot bakgrunnen uten egen farge på tegnet inne i sirkelen :)

@ivarni
Copy link
Contributor

ivarni commented Jan 24, 2025

Hva gjør vi med disse?

https://github.com/fremtind/jokul/blob/main/packages/list/list.scss#L14-L21

Vi kunne kanskje erstattet bakgrunnsbildene i ::before pseudo-elementene med selve unicode punktet som content og så satt fonten til ikon-font?

@piofinn
Copy link
Contributor

piofinn commented Jan 24, 2025

Hva gjør vi med disse?

Vi kunne kanskje erstattet bakgrunnsbildene i ::before pseudo-elementene med selve unicode punktet som content og så satt fonten til ikon-font?

Ja, det skal funke. Hvis ikonet kommer fra fonten kan man vel til og med bruke list-style-type og @counter-style uten å måtte styre med ::before.

Edit: ::marker er kanskje enda bedre! Ser dessverre ut som det mangler litt støtte for vertikal plassering. For kompatibilitet med Safari må man nok fortsatt sette tegnet med list-style-type, men man kan styre font og farge med ::marker.

Edit 2: Funker dårlig å styre posisjon med list-style og ::marker. Fikk det ikke til å se bra ut. Men det funker helt fint med unicode-tegn i ::before, i hvert fall:
Skjermbilde 2025-01-24 kl  11 27 27

Vi vil uansett trenge en fornuftig farge i så fall.

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

Successfully merging this pull request may close these issues.

Feil: Ikoner for checkmarks er ikke like
4 participants