-
Notifications
You must be signed in to change notification settings - Fork 11
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(Vanilla Extract): as a future replacement of jss #561
Conversation
Size stats
|
Deploy preview for mistica-web ready! ✅ Preview Built with commit 1185096. |
Accessibility report ℹ️ You can run this locally by executing |
@@ -18,6 +18,7 @@ storybook-static/ | |||
.vercel | |||
src/generated/mistica-icons/index.tsx.txt | |||
reports | |||
css/mistica.css |
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 file is generated by yarn build
className={classes.avatar} | ||
role="img" | ||
aria-label={ariaLabel ?? initials} | ||
style={{width: size, height: size, color: textColor, background: backgroundColor}} |
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.
so now we will use more and more inline styles to support things like 'isInverse' colors right? or could we create a common set of styles with sprinkles for that?
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.
we can use sprinkles when possible (as an alternative to inline styles), but only when dynamic values are available in our sprinkles definitions. For example, if a component receives a prop color
, we can use it with sprinkles if the color is one of the skin, but if the color
prop accepts any string then we need to use inline styles
|
||
const sizes = [0, 2, 4, 8, 12, 16, 24, 32, 40, 48, 56, 64, 72, 80] as const; | ||
|
||
const commonProperties = defineProperties({ |
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.
from the documentation
conditions: {
mobile: {},
tablet: { '@media': 'screen and (min-width: 768px)' },
desktop: { '@media': 'screen and (min-width: 1024px)' }
}
i think this is also interesting
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 still need to figure out how to work with mediaqueries, because until now mistica media queries have been configurable with context, but I don't know how to make them configurable and at the same time be able to use them in vanilla extract / sprinkles. (css doesn't allow variables in @media
😞 )
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've added a TODO to the PR list
really nice, two questions
|
Classnames are generated at build time, so there should never be hydration missmatches
As said in "Release plan", once we migrate all components to VS, we'll remove JSS from mistica, so the apps using mistica which were using |
const getMediaQueriesConfig = () => { | ||
const impossibleSize = 999999; | ||
|
||
if (process.env.FORCE_MOBILE) { | ||
return { | ||
tabletMinWidth: impossibleSize, | ||
desktopMinWidth: impossibleSize, | ||
largeDesktopMinWidth: impossibleSize, | ||
desktopOrTabletMinHeight: impossibleSize, | ||
}; | ||
} | ||
if (process.env.FORCE_DESKTOP) { | ||
return { | ||
tabletMinWidth: 0, | ||
desktopMinWidth: 0, | ||
largeDesktopMinWidth: impossibleSize, | ||
desktopOrTabletMinHeight: 0, | ||
}; | ||
} | ||
return { | ||
desktopOrTabletMinHeight: 0, | ||
}; | ||
}; | ||
|
||
const getOutputPath = () => { | ||
if (process.env.FORCE_MOBILE) { | ||
return './public/playroom-mobile/'; | ||
} | ||
if (process.env.FORCE_DESKTOP) { | ||
return './public/playroom-desktop/'; | ||
} | ||
return './public/playroom/'; | ||
}; |
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.
maybe this file could include a comment explaining the reasons of this, as stated in the PR description
where is done the double build that you talked about in the description? I cant see it @atabel |
see |
@@ -15,6 +15,7 @@ export { | |||
ServerSideStyles, | |||
} from './jss'; | |||
|
|||
export {vars as skinVars} from './skins/skin-contract.css'; |
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.
Export vars (skin colors) to be usable by apps using Vanilla Extract
vite.config.js
Outdated
// Change .css.js files to something else so that they don't get re-processed by consumer apps using vanilla extract too | ||
fileNames: ({name}) => `${name.replace(/\.css$/, '.css.vanilla')}.js`, |
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.
Vanilla Extract Next plugin applies to node_modules too, so it tries to re-process mistica files (which are already compiled by mistica build) and result in errors. With this change, we make those files invisible for the Next plugin in apps using mistica and VE (zeus, for example).
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.
perhaps a better name would be .css.mistica
react(), | ||
vanillaExtractPlugin(), | ||
noBundlePlugin({ | ||
// Change .css.js files to something else so that they don't get re-processed by consumer apps using vanilla extract too |
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 is because VE process all files ended in css
? ir is because of vite?
# [12.9.0](v12.8.0...v12.9.0) (2022-11-10) ### Bug Fixes * **Cards:** close button ([#576](#576)) ([4c27059](4c27059)) * **playroom:** PreviewTools tabs in desktop ([#574](#574)) ([76f39cc](76f39cc)) ### Features * **Vanilla Extract:** as a future replacement of jss ([#561](#561)) ([8e54a95](8e54a95))
🎉 This PR is included in version 12.9.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Tasks
To do:
mistica.css
file.css.ts
extensions@telefonica/eslint-config
box-sizing: border-box
ThemeConfig['mediaQueries']
and will be removed in future versionsThemeConfig['mediaQueries']
configuration in Playroom for two things: overridedesktopOrTabletMinHeight
to 0 to not fallback to mobile break point when the bottom code panel is shown, and for thePreviewTools
forceMobile
andforceDesktop
props. Those props are heavily used in Mistica docs in brand factory site. To support these use cases the solution I've found is making two additional Playroom static builds served in/playroom-desktop
and/playroom-mobile
which use custom media query breakpoints (seeplayroom.config.js
changes). The downside is that this will increase the preview deployments time. A future improvement could be to only make these additional playroom builds when deploying to prod, and skip them in PR previews.Nice to have:
Release plan
We can release a first version with all the setup and then migrate component by component in future PRs
Major version?
Some breaking changes:
createUseStyles
function, so we'll need a major version for sure. But this isn't done in this PR yet and we can do it in future PRs once we have migrated all componentscss/mistica.css
file. This is a breaking change. We could find a way to mitigate this by inlining the css in the js or something. It isn't the more performance solution but could be a good transient solution that doesn't force us to release a major version yetThe plan:
<style>
tag byThemeContextProvider
.ThemeConfig['mediaQueries']
(see details above).createUseStyles
createUseStyles
from mistica