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

Replace the design system with local source files #171

Merged
merged 10 commits into from
Feb 2, 2024

Conversation

yannbf
Copy link
Contributor

@yannbf yannbf commented Feb 1, 2024

Second attempt at fixing the issue, this time with a bit more harsh measure: copy the files used from @storybook/design-system, and get rid of the dependency altogether (for now at least)

Turns out this fixed the issue and it runs OK on a project:

image

But there are consequences locally that caused CI failures and such.

👉 Please make sure to test the canary really well before merging

📦 Published PR as canary version: 1.0.3--canary.171.b614185.0

✨ Test out this PR locally via:

npm install @chromatic-com/[email protected]
# or 
yarn add @chromatic-com/[email protected]

@yannbf yannbf added release Auto: Create a `latest` release when merged patch Auto: Increment the patch version when merged labels Feb 1, 2024
@yannbf yannbf force-pushed the fix/replace-design-system branch from 2f5efdd to 6b38611 Compare February 1, 2024 18:32
MutableRefObject,
} from 'react';
import { styled, css } from '@storybook/theming';
import { WithTooltip, TooltipMessage } from '@storybook/components';
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 was changed from

import { WithTooltip, TooltipMessage } from '@storybook/design-system';

and I do not know whether there is a big difference, whether this will break or not. Please test it well

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

@andrewortwein andrewortwein left a comment

Choose a reason for hiding this comment

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

Looks good 👍🏻

- The previous template string styles allowed some bad patterns, mixing static and dynamic styles and indescriminately overwriting styles for a selector
- When converting those styles to JS objects, I unwittingly copied those patterns, which now caused issues
- To fix
    - I separated (where reasonable) static and dynamic (those that depend on `props`) styles into multiple style arguments
    - I ensured that all top-level selector object keys are only ever in the styles once and all of the conditionality happens within
@kylegach kylegach merged commit 9c056a4 into main Feb 2, 2024
5 of 6 checks passed
@kylegach kylegach deleted the fix/replace-design-system branch February 2, 2024 00:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
patch Auto: Increment the patch version when merged release Auto: Create a `latest` release when merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants