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

feat: scope option for libraries #369

Closed
wants to merge 2 commits into from

Conversation

chrisbateman
Copy link

Summary

see #299, #208, #178 and updates to docs/CONFIGURATION.md.

I wasn't sure what kind of API you had in mind for this feature, so I won't be offended if you want to go in a different direction!

Test plan

Added some Jest tests, linked locally and tested scope: false/true/"custom" and verified that different class names were generated for each.

@chrisbateman chrisbateman force-pushed the scope branch 2 times, most recently from 53206bb to bac422f Compare March 31, 2019 22:00
@callstack-bot
Copy link

callstack-bot commented Mar 31, 2019

Hey @chrisbateman, thank you for your pull request 🤗.
The coverage report for this branch can be viewed here.

Copy link
Member

@satya164 satya164 left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. Looks good to me. Any harm if we do this by default instead of accepting an option?

My scope idea was to prefix the class name in the generated CSS, like .my-library .cs56ghgd { ... }, so probably shouldn't use scope for the config name, but I also wanted what you did in the PR.

cc @thymikee @zamotany

@chrisbateman
Copy link
Author

Gotcha – that'd work for me! I could make that update later today – any suggestions for the config name? prefix? classPrefix?

@chrisbateman
Copy link
Author

@satya164 Let me know if this is more like what you were thinking of!

@thymikee
Copy link
Member

Just a heads-up: we're working on TS migration in #398 and will let you know once we're done. Would you be so kind and bear with us until it's ready and rebase then?

@pbitkowski
Copy link
Contributor

@satya164 can we merge it after TS migration? Could you take a look?

@chrisbateman
Copy link
Author

@thymikee can do - thanks for the update!

@chrisbateman
Copy link
Author

chrisbateman commented Jul 23, 2019

@thymikee @satya164 Would there be any value in pointing this PR at the develop branch? Or are y'all planning to merge that to master at some point?

@jayu jayu added the cat: improve publishing 🚢 Issues about improvement of publishing libs with Linaria label Apr 1, 2020
@jayu
Copy link
Contributor

jayu commented Apr 1, 2020

Hi, @chrisbateman Do you have the capacity and will to refactor this to the point it would be possible to merge it to current master?

@jayu jayu added the needs: response 📩 Response from author/commenters is requested label Apr 1, 2020
@amaranth
Copy link

How is this different from setting the existing classNameSlug setting to "MyScope_[hash]"?

@jayu
Copy link
Contributor

jayu commented Apr 23, 2020

Lol I didn't notice that :o Looks like classNameSlug PR was introduced after this one was opened. So this PR does not make sense anymore since we have the possibility to add a prefix to the class name. Thanks for that finding @amaranth

@jayu jayu closed this Apr 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cat: improve publishing 🚢 Issues about improvement of publishing libs with Linaria needs: response 📩 Response from author/commenters is requested
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants