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] Nested token-parsing #1347

Merged
merged 6 commits into from
Nov 17, 2021
Merged

[FIX] Nested token-parsing #1347

merged 6 commits into from
Nov 17, 2021

Conversation

KenAJoh
Copy link
Collaborator

@KenAJoh KenAJoh commented Nov 16, 2021

Style-dictionary støtter ikke strukturer med nestede verdier + default verdier (amzn/style-dictionary#643 (comment))
Dette gjør at eks dette oppsettet ikke fungerer og SD ignorerer subtle her.

"hover": {
  "subtle": {
    "value": "{navds.global.color.blue.50.value}"
  },
   "value": "{navds.global.color.blue.600.value}"
}

Foreslått fix er at alle nivå over det "dypeste" blir prefikset med @. Så eks eksemplet over blir da:

"hover": {
  "subtle": {
    "value": "{navds.global.color.blue.50.value}"
  },
  "@": {
   "value": "{navds.global.color.blue.600.value}"
  }
}

Style dictionary ignorerer alle special-characters og resultatet blir da

--navds-global-color-interaction-hover: ...
--navds-global-color-interaction-hover-subtle : ...

Slipper da å rename eks navds-global-color-interaction-primary-hover -> navds-global-color-interaction-primary-hover-default bare for å støtte parsing med SD.
Er en litt hacky løsning som er litt kjip å måtte løse slik, så ser gjerne at vi finner en bedre måte å løse det på. Satser på at SD fikser dette i v4 etterhvert.

Alternativ løsning

Vil også være mulig å å sjekke om en verdi er "dypere" enn en default value, for så å ikke kjøre "deepen" for den "dypere" verdien 😵‍💫 og flytter den ut et hakk. Eksemplet viser kanskje bedre (bruker første eksemplet som input):

"hover": {
   "value": "{navds.global.color.blue.600.value}"
},
"hover-subtle": {
  "value": "{navds.global.color.blue.50.value}"
}

@andnorda
Copy link
Collaborator

🤮

En kjedelig konsekvens av dette er at det er stress å referere til interaction-hover i style dictionary. Blir det da eventuelt sånn: {navds.semantic.color.interaction.hover.@.value} eventuelt {navds.semantic.color.interaction.hover-subtle.value}? Jeg vet ikke om det egentlig er så veldig relevant for oss nå, men tenkte det var verdt å nevne.

@KenAJoh
Copy link
Collaborator Author

KenAJoh commented Nov 17, 2021

🤮

En kjedelig konsekvens av dette er at det er stress å referere til interaction-hover i style dictionary. Blir det da eventuelt sånn: {navds.semantic.color.interaction.hover.@.value} eventuelt {navds.semantic.color.interaction.hover-subtle.value}? Jeg vet ikke om det egentlig er så veldig relevant for oss nå, men tenkte det var verdt å nevne.

Jepp er akkurat det som skjer, så er kjipt uansett hvilken løsning vi velger...

@andnorda
Copy link
Collaborator

Ødelegger hover-subtle output på andre formater enn kebab-case? Blir det da feks NavdsGlobalColorInteractionHover-subtle i js?

@KenAJoh
Copy link
Collaborator Author

KenAJoh commented Nov 17, 2021

Ødelegger hover-subtle output på andre formater enn kebab-case? Blir det da feks NavdsGlobalColorInteractionHover-subtle i js?

Nei alle formater untatt cjs blir som ønsket:

// css
--navds-semantic-color-text-link: var(--navds-global-color-blue-500);
--navds-semantic-color-text-link-visited: var(--navds-global-color-purple-500);


// js
export const NavdsSemanticColorTextLinkVisited = "rgba(99, 70, 137, 1)";
export const NavdsSemanticColorTextLink = "rgba(0, 103, 197, 1)";

// less
@navds-semantic-color-text-link-visited: rgba(99, 70, 137, 1);
@navds-semantic-color-text-link: rgba(0, 103, 197, 1);

// Scss
$navds-semantic-color-text-link-visited: rgba(99, 70, 137, 1);
$navds-semantic-color-text-link: rgba(0, 103, 197, 1);

// cjs, blir ikke som ønsket her
"link-visited": {
	"value": "rgba(99, 70, 137, 1)",
},
"link": {
	"value": "rgba(0, 103, 197, 1)",
 },

@andnorda
Copy link
Collaborator

Hva med for "@". Blir output riktig da?

@KenAJoh
Copy link
Collaborator Author

KenAJoh commented Nov 17, 2021

Hva med for "@". Blir output riktig da?

Alle fungerer untatt Cjs her også. For Cjs får man da

link: {
"@": {...},
"visited": {...}
}

@KenAJoh
Copy link
Collaborator Author

KenAJoh commented Nov 17, 2021

Ser ikke ut som noen bruker cjs versjonen idag i NAV. Kan evt vurdere å bare ikke tilby det formatet for nå.

@andnorda
Copy link
Collaborator

Ja, det høres lurt ut.

@andnorda
Copy link
Collaborator

Det jeg liker med "@" over bindestrek er at vi ikke trenger å bryte opp strukturen på input.

@KenAJoh
Copy link
Collaborator Author

KenAJoh commented Nov 17, 2021

Ah fant en fix på det! Bruker modulen javascript/module-flat og ikke javascript/module for å generere Cjs.
Får da formatet:

"NavdsSemanticColorTextLink": "rgba(0, 103, 197, 1)",
"NavdsSemanticColorTextLinkVisited": "rgba(99, 70, 137, 1)",

noe som matcher Esm formatet

export const NavdsSemanticColorTextLink = "rgba(0, 103, 197, 1)";
export const NavdsSemanticColorTextLinkVisited = "rgba(99, 70, 137, 1)";

@andnorda
Copy link
Collaborator

Jeg stemmer for "@", selv om det er en stygg hack

@KenAJoh
Copy link
Collaborator Author

KenAJoh commented Nov 17, 2021

Ok, kan bare merge denne PR-en da hvis det ikke er noe mer å justere i koden 🚀

@KenAJoh KenAJoh merged commit 9338995 into master Nov 17, 2021
@KenAJoh KenAJoh deleted the tokens-parsing-patch branch November 17, 2021 14:10
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