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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Point style field to built css #394

Merged

Conversation

koddsson
Copy link
Contributor

@koddsson koddsson commented Nov 8, 2017

Hey! 馃憢

I want to be able to import primer packages like this in my project:

import 'primer-alerts'

and have it setup to import the file set in the style field in my project for primer packages but in a lot of packages it points to the SASS file and I much rather have it point to the built CSS so I don't have to compile it myself.

/cc @primer/ds-core

@koddsson koddsson force-pushed the point-style-field-to-built-css branch from df49ea2 to 409ed43 Compare November 8, 2017 13:28
Copy link
Contributor

@josh josh left a comment

Choose a reason for hiding this comment

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

馃 I could see some people wanting the SCSS version so they can access variables.

Is there some convention for specifying these both separately so the build tool will use the appropriate one? Like JS has main and module for ES5 vs ES6+.

@koddsson
Copy link
Contributor Author

koddsson commented Nov 9, 2017

Is there some convention for specifying these both separately so the build tool will use the appropriate one? Like JS has main and module for ES5 vs ES6+.

If there isn't we can probably make our own with something like "style:css" and "style:scss" and then fish out the fields we want in the build process.

@josh
Copy link
Contributor

josh commented Nov 9, 2017

To add some additional context, I was experimenting with webpack's style importing and noticed the full path had to be used.

import styles from "primer-buttons/build/build.css";

https://gist.github.com/josh/e93fc4491e327307cd0ac1681e7d7066

@muan
Copy link
Contributor

muan commented Nov 10, 2017

For reference: Bootstrap uses sass and style. Also found a few others using *.scss as main.

@broccolini
Copy link
Member

We discussed this at our last sync, and are going to address this in the next minor release because we didn't want to add more changes to v10 (shipping today). On a quick look it seems that pointing style to the built CSS is popular, but we agree providing a key for the scss makes sense too, so we'll do some more research but main and sass seem like sensible suggestions.

@broccolini broccolini added this to Release backlog in 馃摝 Primer CSS release tracking via automation Nov 21, 2017
@broccolini broccolini moved this from Release backlog to 馃檰 10.2.0 To Do in 馃摝 Primer CSS release tracking Nov 21, 2017
@jonrohan jonrohan mentioned this pull request Nov 21, 2017
12 tasks
Copy link
Member

@broccolini broccolini left a comment

Choose a reason for hiding this comment

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

Thanks @koddsson! Going to merge this in for Primer 10.2 and will add another option for the Sass.

@broccolini broccolini changed the base branch from dev to release-10.2.0 December 7, 2017 20:17
@broccolini broccolini merged commit 6eed484 into primer:release-10.2.0 Dec 7, 2017
馃摝 Primer CSS release tracking automation moved this from 馃檰 10.2.0 To Do to 馃挏 Merged Dec 7, 2017
@koddsson koddsson deleted the point-style-field-to-built-css branch December 8, 2017 12:45
astorije added a commit to thelounge/thelounge that referenced this pull request Dec 29, 2017
crosma pushed a commit to crosma/lounge that referenced this pull request Jan 8, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

5 participants