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

Fix svelte example components to not add css global #19410

Closed
wants to merge 6 commits into from

Conversation

Amerlander
Copy link
Contributor

Issue: #19409

What I did

I moved the imported css directly into the component to scope it to the component and prevent it from being applied global.

How to test

  • Is this testable with Jest or Chromatic screenshots?

I'm not into jest, I guess this is testable, but I don't know how to add these tests.

@benmccann
Copy link
Contributor

Should we add a comment at least saying where it came from so that we can keep it in sync more easily? And maybe also in the source file letting people know if they update it they need to do so in both places

@Amerlander Amerlander changed the title Fix svelte example components to add global css Fix svelte example components to not add css global Oct 10, 2022
@yannbf yannbf added maintenance User-facing maintenance tasks cli svelte labels Oct 10, 2022
@IanVS
Copy link
Member

IanVS commented Oct 10, 2022

Thanks for identifying this issue and for the PR. We talked about it a bit as a team, and wonder if you would be willing to solve this within the .css files themselves, by properly scoping them to the components (the other option you identified in the issue you opened). This would have the benefit of solving the issue for all frameworks, and would avoid duplication and drift in the svelte examples. Is that something you'd be willing to consider doing?

@Amerlander
Copy link
Contributor Author

Well, haven't thought about that, but I think that makes sense.

The other option I identified in the issue was like using sass to import the .css files inside the style tags – but that would require to add scss as a permanent dependence. If I got you right, you mean, that I just add classes in the .css to each rule and to each component (so not only for svelte, but for each target), like doing the scoping manually.

I'll close this and will come up with a new PR the next days.

@Amerlander Amerlander closed this Oct 10, 2022
@IanVS
Copy link
Member

IanVS commented Oct 10, 2022

Thanks, yes I think we want to keep this as simple as possible, and not introduce new dependencies. I think that adding a class to the component and scoping the css to that class is the best way to go here. The button.css already uses that approach, with a storybook-button class.

I understand if this is maybe more work than you bargained for, but super appreciate if you're willing to take a stab at it. Feel free to open a draft PR with one framework to gather feedback before going through all of them, if you'd like.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cli maintenance User-facing maintenance tasks svelte
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants