-
Notifications
You must be signed in to change notification settings - Fork 40
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
refactor(color): 🏷️ better type safety using colors in code #3176
Conversation
|
Preview deployments for this pull request: Storybook - Storefront - Theme - |
Coverage Report
File CoverageNo changed files found. |
6ac921e
to
bbffcd0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fin rydding! Eg har eit forslag du kan vurdere å titte på også.
@@ -59,7 +59,7 @@ export const ThemePages = () => { | |||
for (let i = 0; i < lightColors.length; i++) { | |||
const number = (i + 1) as ColorNumber; | |||
style[ | |||
`--ds-color-neutral-${getColorInfoFromPosition(number) | |||
`--ds-color-neutral-${getColorMetadataByNumber(number) | |||
.displayName.replace(/\s+/g, '-') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Når du først er i gang, erstatte getColorMetadataByNumber(number).displayName
og så ymse .toLowerCase
og .replace
med kun getColorMetadataByNumber(number).name
? Til tross for littegrann diff i resulterande variabelnamn som eg skriv om under streken, trur eg det skal funke fint 😄
...det blir då samme verdi som før unntatt for base-contrast-subtle
og base-contrast-default
, som har displayname Contrast Subtle
og Contrast Default
(og dermed ikkje hadde base-
prefix etter string-manipulering) 🤔
Det er litt pussig, eg har ikkje sett nøye nok på koden til å skjønne om det er eit poeng, men det virker rart. css-variabelen i våre genererte css-filer heiter jo også base-contrast-subtle
og -default
, så der er temabyggeren ute av sync. Men dersom vi berre fikser dette alle stadene getColorMetadataByNumber(number)
blir brukt så vil det vel sikkert funke?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Helt enig med deg :) Var ikke helt klar over akkurat denne kodesnutten selv før idag. Har tenkt å se på en fiks for dette i egen PR da vi har som forventet bugs rundt det idag. https://designsystemet.slack.com/archives/C05U1MNKYCX/p1740059663332569
Tipper vi har nok flere bugs i temabygger relatert til farge da det er bruk indexer flere plasser som id for farger, noe som hverken matcher farge nummeret eller nå som vi endret på skalaen.
Har lenge planlagt at vi skulle gjøre noe med akkurat dette (#2920). Dette er steg i den retningen da jeg har sakt begynt å fikse opp i det nå som vi har hatt litt disponibel tid og faktisk fått bugs på det (som jeg har lenge fryktet kom til å skje).
Co-authored-by: Une Sofie Kinn Ekroll <[email protected]>
In an effort to prevent bugs related to using indexs for fetching arbitary colors I have introduces stricter types for colorMetaData making safer lookups by name or number