-
Notifications
You must be signed in to change notification settings - Fork 1
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
feat: resolve css variables in experiences #596
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
f8e94ff
to
9180d23
Compare
6000927
to
101f92e
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.
LGTM 🥇
const element = document.createElement('div'); | ||
document.body.appendChild(element); |
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.
Does this work with SSR? 🤔
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.
I would also discourage from trying to affect the DOM from within a plain js function.
I think react would expect it to be done in the useEffect
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.
It is compatible with SSR, @Spring3 can you explain what the difference in using the useEffect does? I'm only temporarily creating it to grab the css values and then deleting the element but am open to hearing your thoughts and follow up on your suggestion.
if (sizing) { | ||
const rawResolvedValues = getAllCssVariableValues(element, sizing, 'width'); | ||
Object.assign(resolvedCssVariables, rawResolvedValues); | ||
} | ||
if (color) { | ||
const rawResolvedValues = getAllCssVariableValues(element, color, 'background-color'); | ||
Object.assign(resolvedCssVariables, rawResolvedValues); | ||
} | ||
if (borderRadius) { | ||
const rawResolvedValues = getAllCssVariableValues(element, borderRadius, 'border-radius'); | ||
Object.assign(resolvedCssVariables, rawResolvedValues); | ||
} | ||
if (fontSize) { | ||
const rawResolvedValues = getAllCssVariableValues(element, fontSize, 'font-size'); | ||
Object.assign(resolvedCssVariables, rawResolvedValues); | ||
} | ||
if (lineHeight) { | ||
const rawResolvedValues = getAllCssVariableValues(element, lineHeight, 'line-height'); | ||
Object.assign(resolvedCssVariables, rawResolvedValues); | ||
} | ||
if (letterSpacing) { | ||
const rawResolvedValues = getAllCssVariableValues(element, letterSpacing, 'letter-spacing'); | ||
Object.assign(resolvedCssVariables, rawResolvedValues); | ||
} | ||
if (textColor) { | ||
const rawResolvedValues = getAllCssVariableValues(element, textColor, 'color'); | ||
Object.assign(resolvedCssVariables, rawResolvedValues); | ||
} |
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.
This could be refactored to use a loop to reduce some of the repetition. For example:
const cssProperties = [
{ variable: sizing, property: 'width' },
{ variable: color, property: 'background-color' },
{ variable: borderRadius, property: 'border-radius' },
{ variable: fontSize, property: 'font-size' },
{ variable: lineHeight, property: 'line-height' },
{ variable: letterSpacing, property: 'letter-spacing' },
{ variable: textColor, property: 'color' },
];
cssProperties.forEach(({ variable, property }) => {
if (variable) {
const rawResolvedValues = getAllCssVariableValues(element, variable, property);
Object.assign(resolvedCssVariables, rawResolvedValues);
}
});
packages/core/src/types.ts
Outdated
@@ -228,7 +228,7 @@ export type RecursiveDesignTokenDefinition = { | |||
}; | |||
|
|||
export type DesignTokensDefinition = { | |||
spacing?: Record<string, string>; | |||
spacing?: Record<string, string> | Record<string, number>; |
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.
This would also work here:
spacing?: Record<string, string> | Record<string, number>; | |
spacing?: Record<string, string | number>; |
101f92e
to
44a6e2a
Compare
44a6e2a
to
eda9ff2
Compare
eda9ff2
to
bc91949
Compare
bc91949
to
c8c17dc
Compare
c8c17dc
to
26bc677
Compare
26bc677
to
1c39b78
Compare
1c39b78
to
53f68e0
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.
I'm still not sure if the document.createElement
approach will work in SSR-land, but approving either way and we can look at that later if it comes up.
Purpose
Add css variable support for Experiences for Design Tokens
Approach
Create an in memory resolved css variables values similar to design tokens. That way the ui can consume the values and store it in its immer store and thus make it accessible throughout the ui webapp