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

Add semibold (600) fonts #2150

Merged
merged 6 commits into from
Aug 11, 2023
Merged

Add semibold (600) fonts #2150

merged 6 commits into from
Aug 11, 2023

Conversation

cskrov
Copy link
Contributor

@cskrov cskrov commented Aug 10, 2023

Legger til italic og normal semibold (600) versjoner av Source Sans 3.

Lastet ned fra Google Fonts i WOFF2 format.

SourceSans3-italic_semibold-[latin|cyrillic](_ext).woff2, dvs. italic og semibold, er lagt til for å støtte tekst som er både italic og bold.

SourceSans3-normal_semibold-[latin|cyrillic](_ext).woff2, dvs. normal og semibold, er lagt til for å ha korrekt fontversjon for fet skrift. Selv om dette rendret OK fra før.
Ligger 600 som en del av andre fontfilene allerede, eller har nettleserne emulert 600 godt nok?

Fjernet SourceSans3-normal_semibold-[latin|cyrillic](_ext).woff2, dvs. normal og semibold, se kommentar fra @KenAJoh under. Foretrekker variabel font rendering over ekstra kB.

Uten semibold italic font

uten-semibold-italic-font

Med semibold italic font

med-semibold-italic-font

@changeset-bot
Copy link

changeset-bot bot commented Aug 10, 2023

🦋 Changeset detected

Latest commit: 87b1a07

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 7 packages
Name Type
@navikt/ds-css Patch
@navikt/aksel-stylelint Patch
@navikt/aksel Patch
@navikt/ds-react Patch
@navikt/ds-tokens Patch
@navikt/ds-tailwind Patch
@navikt/aksel-icons Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@KenAJoh
Copy link
Collaborator

KenAJoh commented Aug 10, 2023

Hei 👋 I teorien skal det ikke være nødvendig å ha flere fonter da Source sans 3 er en variabel font.

Testet litt diverse kombinasjoner i en enkel codesandbox her og virker som fontene justeres riktig basert på font-weight og italic
400
Screenshot 2023-08-10 at 10 19 25
400 italic
Screenshot 2023-08-10 at 10 19 29
600
Screenshot 2023-08-10 at 10 19 35
600 italic
Screenshot 2023-08-10 at 10 19 40

Kan hende de statiske versjonene av fonten er litt bedre, men koster da 10-40+kB ekstra 😞

@cskrov
Copy link
Contributor Author

cskrov commented Aug 10, 2023

Ok, for oss er forskjellen betydelig og problematisk 😞

Normal italic vs semibold italic

normal semibold

Det er tilnærmet ingen forskjell på disse to eksemplene. Som er uakseptabelt for våre brukere.

Selv i de betydelig større eksemplene over må jeg se nøye etter, og hadde ikke klart å se forskjell på 400 italic og 600 italic om de ikke sto ved siden av hverandre.

Det sagt, så er våre brukere interne og vi kan ev. overstyre fontene fra Aksel.

Til sammenligning, her er samme tekst med statisk versjon
uten-semibold-italic-font med-semibold-italic-font

@cskrov cskrov force-pushed the add-semibold-fonts branch from 785bb71 to 40fd826 Compare August 10, 2023 09:01
@KenAJoh
Copy link
Collaborator

KenAJoh commented Aug 10, 2023

Ok, for oss er forskjellen betydelig og problematisk 😞

Normal italic vs semibold italic

normal semibold
Det er tilnærmet ingen forskjell på disse to eksemplene. Som er uakseptabelt for våre brukere.

Selv i de betydelig større eksemplene over må jeg se nøye etter, og hadde ikke klart å se forskjell på 400 italic og 600 italic om de ikke sto ved siden av hverandre.

Det sagt, så er våre brukere interne og vi kan ev. overstyre fontene fra Aksel.

Til sammenligning, her er samme tekst med statisk versjon uten-semibold-italic-font med-semibold-italic-font

Hvilken nettleser og OS er brukt her? Hvis alt fungerer som det skal bør det se likt ut for alle, men kan hende det er browser/OS-spesifikt 🤔 Er dette live i prod som man kan teste eller kanskje devmiljø?

@cskrov
Copy link
Contributor Author

cskrov commented Aug 10, 2023

Ja, det er stor forskjell på hvordan Firefox og Chrome håndterer variable fonter ser det ut til.

Vi ser problemet på MacOS og Linux.

Firefox

firefox

Chrome

chrome

Rekkefølgen på variasjonene over er

  1. 400 normal
  2. 600 normal
  3. 400 italic
  4. 600 italic

Modifiserte CodeSandboxen din for å teste Chrome vs. FF mens du skrev kommentaren din :P
https://codesandbox.io/s/dazzling-hoover-88lszt?file=/App.js

@KenAJoh
Copy link
Collaborator

KenAJoh commented Aug 10, 2023

Har oppdatert CSS for font-setting da 👍 Trenger bare vanlig latin for 99% av tilfellene, men trenger dere ext og cyrillic også?

@github-actions
Copy link
Contributor

github-actions bot commented Aug 10, 2023

f23714d41 | 48 komponenter | 307 stories

@cskrov
Copy link
Contributor Author

cskrov commented Aug 10, 2023

Supert 👍
Venter på svar fra saksbehandlerne våre før jeg svarer helt sikkert på det.
Er ikke umulig at de benytter alle slags tegn, spesielt ift. navn, men jeg vet ikke ennå.

@cskrov
Copy link
Contributor Author

cskrov commented Aug 10, 2023

Svaret var ja, de bruker noen ganger tegn fra andre tegnsett.

Jeg så det allerede lå kyrillisk og latin ext. inne, så andre tegnsett burde være nyttig for noen andre også?

@KenAJoh
Copy link
Collaborator

KenAJoh commented Aug 10, 2023

I de tilfellene latin-ext eller kyrillisk brukes vil fonten bli erstattet av Arial som har god støtte for italic og semibold nå. Vil ikke se helt perfekt ut, men tror det vil være godt nok i de edgecasene der man har italic + semibold. Slipper da å laste inn 30kB ekstra CSS

font-family: "Source Sans Pro", Arial, sans-serif;

@KenAJoh KenAJoh merged commit 0de8238 into main Aug 11, 2023
@KenAJoh KenAJoh deleted the add-semibold-fonts branch August 11, 2023 07:02
@github-actions github-actions bot mentioned this pull request Aug 11, 2023
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.

2 participants