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

Icon updates #974

Merged
merged 5 commits into from
Feb 4, 2021
Merged

Icon updates #974

merged 5 commits into from
Feb 4, 2021

Conversation

andnorda
Copy link
Collaborator

@andnorda andnorda commented Feb 3, 2021

Vet ikke om det er en god ide, men nå settes height, width og fill i svgr istedenfor når de lastes ned fra figma.

@andnorda andnorda requested a review from a team as a code owner February 3, 2021 16:33
@KenAJoh
Copy link
Collaborator

KenAJoh commented Feb 4, 2021

Tenker det er en god endring. Litt usikker på om vi fortsatt bør endre fill ved nedlastning fra figma. Da instedenfor endre fargen til #000 over navMorkGra. Kan ha noen side-effects for #973, men fikser bare det etter denne er live

@andnorda
Copy link
Collaborator Author

andnorda commented Feb 4, 2021

Cool. Ja, jeg var bekymra for at jeg brakk noe, men gadd ikke teste det kl. halv seks i går.

Et annet spørsmål: Hvilke statiske ressurser gir det mening å mellomlagre i git? Figma skal jo være master, men selve svg-ene kan jo være lurt å mellomlagre for å ha litt mer kontroll på hva som endrer seg. Men .zip-filnene er jo generert fra svgene, om jeg skjønner riktig, så de er det kanskje ikke vits å mellomlagre?

@KenAJoh
Copy link
Collaborator

KenAJoh commented Feb 4, 2021

Akkurat nå blir svg.zip generert når ikonene lastes ned, ikke når de blir prosessert med yarn boot så er mellomlagret av den grunn. Men tenker vi kan flytte den jobben til build steget istedenfor så slipper vi det.

https://github.com/navikt/nav-frontend-moduler/blob/af98a451828223866f551899e67b3c7b0bfa7ad7/%40navikt/ds-icons/figma-api/index.js#L108-L110

@andnorda
Copy link
Collaborator Author

andnorda commented Feb 4, 2021

Kult. Jeg kan gjøre den endringen i neste PR

Copy link
Collaborator

@KenAJoh KenAJoh left a comment

Choose a reason for hiding this comment

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

lgtm

@KenAJoh KenAJoh merged commit 527a892 into master Feb 4, 2021
@KenAJoh KenAJoh deleted the icon-updates branch February 4, 2021 09:43
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.

None yet

2 participants