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

[Storybook] Update stitches + conventions #1048

Merged
merged 11 commits into from
Apr 14, 2022
Merged

Conversation

benoitgrelard
Copy link
Contributor

@benoitgrelard benoitgrelard commented Dec 16, 2021

🔥 NOTE: I have --no-verify'ed this for now to put the PR up but, as discussed on Slack, the pre-push is not happy with the stitches types… However, everything is fine on CI, build, storybook build, chromatic, etc

I've called out the meaningful changes I've made, but apart from those, it's mainly just a lot of replace which I explain below.

This PR does a few things:

  • Update stitches to latest (also switch to @stitches/core as we were only using the core for a while anyway)
    • biggest diff was that stitches class need to be called now (), thankfully we were consistent with our naming so I was able to do this with a regex replace 😀
    • tokens in theme shouldn't start with $ anymore
    • keyframes is a new export
    • color tokens were used in boxShadow but aren't mapped now, so had to update to $colors$…
  • Use dotted syntax and import * as … for all multi-part primitives consistently

@@ -466,7 +455,7 @@ const itemClass = css({
});

const subTriggerClass = css(itemClass, {
'&[data-state="open"]': {
'&:not(:focus)[data-state="open"]': {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the only meaningful change I had to make after checking every single story manually. It seems we were relying on some specificity bug from stitches before.

@@ -521,7 +512,7 @@ const animatedOverlayClass = css(overlayClass, {

const animatedContentClass = css(contentDefaultClass, {
'&[data-state="open"]': {
animation: `${fadeIn} ${scaleIn} 300ms ease-out`,
animation: `${fadeIn} 150ms ease-out, ${scaleIn} 200ms ease-out`,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is another change I made, not sure if it ever worked, but I noticed it definitely didn't with this new version (ie. you need 2 separate animations, comma separated).

@benoitgrelard benoitgrelard marked this pull request as ready for review December 16, 2021 19:27
@andy-hook andy-hook self-assigned this Apr 11, 2022
@andy-hook andy-hook marked this pull request as draft April 13, 2022 11:02
@andy-hook andy-hook force-pushed the storybook-stitches-update branch from 05fc91d to bda4fe3 Compare April 13, 2022 11:55
@@ -73,138 +59,160 @@ export const Modality = () => {
<div
style={{ display: 'flex', alignItems: 'center', justifyContent: 'center', height: '110vh' }}
>
<div style={{ display: 'grid', gridGap: 50 }}>
<div style={{ display: 'grid', gap: 50 }}>
Copy link
Contributor

Choose a reason for hiding this comment

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

gripGap was deprecated long ago and no longer renders with that syntax in latest Stitches

@@ -83,7 +83,7 @@
"replace-in-files": "^3.0.0",
"start-server-and-test": "^1.12.5",
"ts-jest": "^26.2.0",
"typescript": "^3.9.7"
"typescript": "^4.6.3"
Copy link
Contributor

Choose a reason for hiding this comment

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

As suspected, this resolved the type issue.

package.json Outdated
Comment on lines 46 to 48
"@stitches/core": "^1.2.7",
"@storybook/addon-storysource": "^6.4.21",
"@storybook/react": "^6.4.21",
Copy link
Contributor

Choose a reason for hiding this comment

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

@@ -2,7 +2,6 @@
"extends": "./tsconfig.base.json",
"compilerOptions": {
"paths": {
"*": ["node_modules/*"],
Copy link
Contributor

Choose a reason for hiding this comment

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

@@ -549,7 +559,7 @@ const buttonClass = css({
border: '1px solid black',
borderRadius: 4,
background: 'gainsboro',
font: 'inherit',
Copy link
Contributor

Choose a reason for hiding this comment

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

This is the cause of the chromatic diff prompt, for some reason font wasn't rendering in the previous Stitches version (we usually use fontFamily)

I've updated it to use fontFamily just for consistency with the other stories though it's functionally the same now anyway since it was seemingly fixed.

@andy-hook andy-hook marked this pull request as ready for review April 13, 2022 19:12
@andy-hook andy-hook force-pushed the storybook-stitches-update branch from 1166e1a to 763bd26 Compare April 14, 2022 11:34
@benoitgrelard benoitgrelard merged commit 768bc53 into main Apr 14, 2022
@benoitgrelard benoitgrelard deleted the storybook-stitches-update branch April 14, 2022 14:43
luisorbaiceta pushed a commit to luisorbaiceta/primitives that referenced this pull request Jul 21, 2022
* Upgrade Stitches

Co-authored-by: Benoît Grélard <[email protected]>

* Upgrade TypeScript

* Upgrade Storybook

* Resolve type issue with Cypress

* Update font consistency in story

* Refresh yarn.lock

* Upgrade dependencies to fix typedefs issues

* Update paths in stories

* Update paths in test files

* Upgrade to storybook next to get React 18 support

* Fix Dialog story and skip genuine test failure

Co-authored-by: Andy Hook <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants