Skip to content
This repository has been archived by the owner on Dec 23, 2017. It is now read-only.

Use local styles conditionally. #478

Merged

Conversation

jmcarp
Copy link
Contributor

@jmcarp jmcarp commented Aug 17, 2015

  • Use styles from local fec-style URL if specified
  • Add a place for vendor styles

* Use styles from local fec-style URL if specified
* Add a place for vendor styles
@jmcarp
Copy link
Contributor Author

jmcarp commented Aug 17, 2015

@noahmanger we should be able to start deleting a bunch of styles from static/styles here. Is there anything we want to keep, other than the two top-level scss files? How about themify-icons?

@@ -0,0 +1 @@
@import "fec-style/scss/styles.scss";
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this being done here rather the /static/styles/styles.scss.

Also, I'm getting confused when things are referencing the local styles or fec-style.

@noahmanger
Copy link
Contributor

Great. Yeah we shouldn't need any of that. And no need to keep themify-icons or the datatables responsive styles

@msecret
Copy link
Contributor

msecret commented Aug 17, 2015

So is this so we can conditionally include fec-styles? And then also have local styles to override?

@msecret
Copy link
Contributor

msecret commented Aug 17, 2015

@noahmanger are you adding datatables to fec-style?

@noahmanger
Copy link
Contributor

The conditional include is so that you can be developing off fec-styles locally and this pulls it in from a local server, rather than having to merge styles in and pull down via npm.

And I think vendor styles like datatables that are only going to be used here can just be included here, separate from fec-style.

@noahmanger
Copy link
Contributor

This works for me. Can you just add a line to the README?

@noahmanger
Copy link
Contributor

Just caught something. It looks like maybe the datatables styles are being loaded in after fec-styles, because its now overriding several things that it wasn't before.

@jmcarp
Copy link
Contributor Author

jmcarp commented Aug 17, 2015

@noahmanger try re-running npm install and npm run build. I'm wondering whether you installed the styles with npm before you merged your latest changes to fec-style.

@noahmanger
Copy link
Contributor

Ooooh, yeah, I was just running them locally because we haven't merged them
in yet.

On Mon, Aug 17, 2015 at 3:47 PM, Joshua Carp notifications@github.com
wrote:

@noahmanger https://github.com/noahmanger try re-running npm install
and npm run build. I'm wondering whether you installed the styles with
npm before you merged your latest changes to fec-style.


Reply to this email directly or view it on GitHub
#478 (comment).

Noah Manger
18F http://18f.gsa.gov | Work: 415-696-6146

* Document style environment configuration in README
* Delete unused imports
* Delete unused styles
@jmcarp
Copy link
Contributor Author

jmcarp commented Aug 17, 2015

Updated: lots of files deleted, and docs updated.

@msecret
Copy link
Contributor

msecret commented Aug 17, 2015

@noahmanger would we need access to the sass variables from fec-style in the local scss files? Like lets say web app wants to override panel from the style guide but needs the $primary variable?

@noahmanger
Copy link
Contributor

I guess if you wanted to do that it would be helpful, but I think even then I'd say those changes should live in fec-style. But I could be convinced.

@@ -7,8 +7,13 @@

{% include 'partials/meta-tags.html' %}

<!-- <link rel="stylesheet" href="/{{ assets['dist/styles/styles.css'] }}" /> -->
<link rel="stylesheet" href="http://localhost:8080/css/styles.css" />
<link rel="stylesheet" href="/{{ assets['dist/styles/styles.css'] }}" />
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this the same thing as line 15?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, this is the vendor / custom styles. Lines 12-16 adds fec-style. We can change the names if you want.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh got it. That works for me.

@noahmanger
Copy link
Contributor

I vote for merging this as-is and if we need to figure out a way to share sass variables we can figure it out then.

noahmanger pushed a commit that referenced this pull request Aug 18, 2015
@noahmanger noahmanger merged commit 978d0c0 into fecgov:feature/new-styles Aug 18, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants