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.

Sign up for GitHub

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

Jump to bottom

fix: error at color-system story #378

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

fix: error at color-system story #378

wants to merge 2 commits into from

Conversation

shootermv
Copy link
Contributor

@shootermv shootermv commented Oct 14, 2024 •
edited by huyenltnguyen
Loading

not sure why, but things start working after renaming color.css to colors.module.css:

Closes #66

@shootermv shootermv requested a review from a team as a code owner October 14, 2024 14:10
huyenltnguyen
huyenltnguyen reviewed Oct 15, 2024
View reviewed changes
Copy link
Member

@huyenltnguyen huyenltnguyen left a comment

Choose a reason for hiding this comment

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

Thank you for the PR.

There are a couple of issues with the changes that I don't think we can merge as-is:

  • colors.css still exists, and its import aren't updated.
  • We are using globally-scoped stylesheets in the codebase, if we are changing this to a module, other stylesheets should follow.
  • We would like to understand the root cause of the issue (whether this is an issue in the repo setup or in the storybook setup) in order to update the config accordingly.

@huyenltnguyen huyenltnguyen added the status: blocked Is waiting on followup from either the Opening Poster of the issue or PR, or a maintainer. label Oct 15, 2024
@shootermv
Copy link
Contributor Author

shootermv commented Oct 15, 2024 •
edited
Loading

Thank you for the PR.

There are a couple of issues with the changes that I don't think we can merge as-is:

  • colors.css still exists, and its import aren't updated.
  • We are using globally-scoped stylesheets in the codebase, if we are changing this to a module, other stylesheets should follow.
  • We would like to understand the root cause of the issue (whether this is an issue in the repo setup or in the storybook setup) in order to update the config accordingly.

i have impression that problem is not the storybook,
seems to me the problem is importing css variables into javascript as object
(import colorList from "../colors.css"; at colors-system component)
for some reason it works with modules an not with genral css...
But! we can use module file only for color-systems and import the colors.css into it using @import ...

@shootermv
Copy link
Contributor Author

is the PR is blocked?

@huyenltnguyen
Copy link
Member

is the PR is blocked?

Yes. The changes haven't addressed my concerns in #378 (review).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@huyenltnguyen huyenltnguyen huyenltnguyen left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
status: blocked Is waiting on followup from either the Opening Poster of the issue or PR, or a maintainer.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Color story is not rendering
2 participants
@shootermv @huyenltnguyen