Converted CSS to SASS #1349

heartsucker · 2016-07-02T14:30:47Z

I wanted to a add new features and CSS and is terrible to work with compared to SASS. This PR includes:

Convert all CSS to SASS
Combine duplicate SASS rules
Add SASS compilation to Ansible / builds

garrettr · 2016-07-05T20:40:34Z

Thanks for this PR, @heartsucker! I've been wanting to do this for a long time, since I agree CSS is terrible to work with compared to alternatives such as SASS. I'll review this by the end of the week.

heartsucker · 2016-11-29T20:36:06Z

Hey guys. This is in need of some rebasing, but before I get to it, can someone take a look and let me know if this seems to be a good candidate for merge?

garrettr

A quick question:

Do you prefer using SASS over SCSS? We're comfortable with either format, but SCSS seems to be slightly more popular these days.

Overall, this PR looks great. If you rebase it and fix the other comments from this review, we will merge it.

garrettr · 2016-12-02T00:59:34Z

securedrop/management/run.py

+                                 'cyan'),
+        lambda: DevServerProcess('SASS Compiler',
+                                 ['sass', '--watch', 'sass:static/css'],
+                                 'magenta'),


Why is it necessary to add lambdas and change DevServerProcessMonitor.__init__? The commit message in 688b747 was unclear.

If one of the subprocesses fails to start or throws and exception, the monitor will leave the other processes dangling. Each call to DevServerProccess needs a try/except around it.

@heartsucker Thanks for the explanation! I'll take a look at this myself - I think there might be a way to more gracefully handle that case within DevServerProcess.

garrettr · 2016-12-02T01:12:16Z

securedrop/sass/source.sass

+     * HACK: Firefox likes to add non-existent leading and/or trailing whitespace
+     * to <span>s, but never does it to <p>s. This hack uses a <p> for the
+     * codename so we can get the right copy behavior, and uses inline-block plus
+     * tweaks the margin to make it look like a span


Close this comment.

garrettr · 2016-12-02T01:14:56Z

securedrop/sass/source.sass

+        font-size: 14px
+
+        p
+            *


Is this line necessary?

Probably not? I'm not a CSS expert, so I made as few changes as possible when porting it to SASS.

@heartsucker Gotcha. Let's leave it in for now, it can be addressed in a follow-up if necessary.

garrettr · 2016-12-02T01:16:16Z

securedrop/source_templates/base.html

-    {% assets filters="cssmin", output="gen/source.css",
-      "css/normalize.css", "css/source.css", "css/font-awesome.css" %}
+
+    {# TODO why doesn't a link to /static/source.css work? #}


We should resolve this TODO before merging. It's best not to leave TODOs in merged code, if possible.

I'll take a look at this since there have been some updates to some of the dependencies since I wrote this and the behavior might have changed. Also, suggestions welcome since this one seems a bit non-intuitive.

garrettr · 2016-12-02T01:16:42Z

...ansible-base/roles/build-securedrop-app-code-deb-pkg/tasks/build_securedrop_app_code_deb.yml

+  args:
+    chdir: /tmp/{{ securedrop_app_code_deb }}/var/www/securedrop/
+#  register: sass_compile_result
+#  changed_when: sass_compile_result.stdout != ''


This code is commented out, can it be removed?

heartsucker · 2016-12-02T10:53:28Z

@garrettr I'm not really a web frontend guy, but more a guy who can do some frontend stuff when he needs. I happened to do it in SASS because that's what I know, but some poking around does seem to indicate that there are more advantages to SCSS. If y'all want that, I can make those change along with all of the rest of this.

psivesely · 2016-12-02T22:45:24Z

I'm not really a web frontend guy

Me either.

poking around does seem to indicate that there are more advantages to SCSS.

From a little research, I saw a decent amount of opinion to that point as well. E.g., http://thesassway.com/editorial/sass-vs-scss-which-syntax-is-better.

If y'all want that, I can make those change along with all of the rest of this.

If that isn't too much work for you, it seems to me like that might be good, just because it seems like SCSS might be better for other contributors to work with in the future. Since I don't do much myself with the frontend codebase myself, I'm not going to make an absolute call one way or the other. Unless one of the other SD team members has strong opinions, we'll leave the decision up to you.

garrettr · 2016-12-02T23:20:55Z

I happened to do it in SASS because that's what I know, but some poking around does seem to indicate that there are more advantages to SCSS. If y'all want that, I can make those change along with all of the rest of this.

@heartsucker I think SASS is fine. I polled some front-end folks that I know and people seem to be pretty evenly split on which one is "better". If we end up deciding to change to SCSS later, that seems to be trivial to do via sass-convert.

heartsucker · 2016-12-03T15:20:53Z

This was tested on the dev VM, but it's not passing Travis because of this mystery:

TASK: [development | Install sass Ruby gem] *********************************** 
<localhost> state=latest name=sass
changed: [localhost] => {"changed": true, "name": "sass", "state": "latest", "version": "3.4.22"}

TASK: [development | Compile sass to css] ************************************* 
<localhost> chdir=/home/travis/build/freedomofpress/securedrop/securedrop
failed: [localhost] => {"changed": true, "cmd": "sass --update sass:static/css", "delta": "0:00:00.004013", "end": "2016-12-03 15:14:05.668050", "rc": 127, "start": "2016-12-03 15:14:05.664037", "warnings": []}
stderr: /bin/sh: 1: sass: not found

It's saying the gem was installed (globally), but then it's not on the path. Anyone have any experience debugging the Travis environment? Also, I added verbosity to build in hopes that it might tell us something, but alas.

This appears to be related: travis-ci/travis-ci#5861

redshiftzero · 2016-12-07T17:06:08Z

Hey @heartsucker, I took a peek at this PR yesterday and it does look to be a weird PATH issue: one way to troubleshoot is to add a couple of post-install tasks in that Ansible play before you try to execute sass, e.g.:

- name: Check sass is in gem list
  command: gem list

- name: Check where gems are being installed
  command: gem environment

Since you put those -vv verbosity flags (good call) you can inspect the output and see what's going on with the PATH. This might not be the Best Way to debug this but it’s what I do ¯\(ツ)/¯. Give that a whirl, if things still look confusing let me know and I’ll take a deeper look.

heartsucker · 2016-12-07T17:12:01Z

@redshiftzero I already did most of that (and them some), but even when putting calls sass with the full path /usr/lib/gems/sass (or whatever it was), it still fails. I've never dealt with Ruby, so whatever seems to be going on with RVM is new to me. Anywho. I can probably get to this this weekend.

heartsucker · 2017-01-15T17:11:03Z

Pinging for review/merge.

heartsucker · 2017-01-18T19:55:41Z

Doubling down on the request for merge because there are a bunch of UI tickets that got opened today, and it would be nice to do this first.

heartsucker · 2017-01-25T15:23:38Z

@garrettr? @fowlslegs? @redshiftzero? Bueller?

conorsch · 2017-01-27T23:12:23Z

Whoa, sorry about the lag on this one, @heartsucker! Thanks for your persistence, getting this in pronto. (Yes, the hack in b9c78be is indeed adventurous approach, but it works well enough for the dev env!)

heartsucker force-pushed the sass branch 7 times, most recently from 7e56a35 to 5dd1d7c Compare July 3, 2016 13:41

heartsucker mentioned this pull request Jul 6, 2016

Make HTML/CSS responsive #1357

Closed

redshiftzero added PR: Needs review labels Nov 3, 2016

psivesely added journalist interface (JI) source interface (SI) labels Nov 5, 2016

psivesely assigned garrettr Nov 14, 2016

garrettr suggested changes Dec 2, 2016

View reviewed changes

heartsucker force-pushed the sass branch 2 times, most recently from a8e974a to 039088b Compare December 3, 2016 14:57

heartsucker force-pushed the sass branch 5 times, most recently from 0914b4b to 35daa9d Compare December 4, 2016 10:27

heartsucker force-pushed the sass branch from 35daa9d to ffcc5ae Compare December 7, 2016 17:33

heartsucker force-pushed the sass branch 8 times, most recently from bab629e to d73e477 Compare December 11, 2016 11:41

psivesely added journalist interface (JI) source interface (SI) labels Dec 16, 2016

heartsucker force-pushed the sass branch from d73e477 to 7f65615 Compare January 15, 2017 17:57

heartsucker and others added 10 commits January 25, 2017 16:06

css -> sass

b4a83d1

deduplicated sass rules

bdbb6ba

added sass compilation into build cycle

6699060

initializing dev server with lambdas to properly register cleanup

6c0deeb

fixed container overflow

90c0449

factored common colors into variables

e8a8edd

fixed errors from sass rebase

4c9aead

added verbosity to travis builds

a371200

removed assets

2ad8678

sass for travis

4e7da7f

heartsucker force-pushed the sass branch from 7f65615 to 4e7da7f Compare January 25, 2017 15:06

conorsch merged commit 0ed80f8 into freedomofpress:develop Jan 27, 2017

heartsucker deleted the sass branch January 28, 2017 15:03

This was referenced Jan 28, 2017

Recommended 0.4 SOURCE UI Fixes #1536

Closed

Ui fixes #1538

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Converted CSS to SASS #1349

Converted CSS to SASS #1349

heartsucker commented Jul 2, 2016

garrettr commented Jul 5, 2016

heartsucker commented Nov 29, 2016

garrettr left a comment

garrettr Dec 2, 2016

heartsucker Dec 2, 2016

garrettr Dec 2, 2016

garrettr Dec 2, 2016

garrettr Dec 2, 2016

heartsucker Dec 2, 2016

garrettr Dec 2, 2016

garrettr Dec 2, 2016

heartsucker Dec 2, 2016

garrettr Dec 2, 2016

heartsucker commented Dec 2, 2016

psivesely commented Dec 2, 2016

garrettr commented Dec 2, 2016

heartsucker commented Dec 3, 2016 •

edited

Loading

redshiftzero commented Dec 7, 2016 •

edited

Loading

heartsucker commented Dec 7, 2016

heartsucker commented Jan 15, 2017

heartsucker commented Jan 18, 2017

heartsucker commented Jan 25, 2017

conorsch commented Jan 27, 2017

Converted CSS to SASS #1349

Converted CSS to SASS #1349

Conversation

heartsucker commented Jul 2, 2016

garrettr commented Jul 5, 2016

heartsucker commented Nov 29, 2016

garrettr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

heartsucker commented Dec 2, 2016

psivesely commented Dec 2, 2016

garrettr commented Dec 2, 2016

heartsucker commented Dec 3, 2016 • edited Loading

redshiftzero commented Dec 7, 2016 • edited Loading

heartsucker commented Dec 7, 2016

heartsucker commented Jan 15, 2017

heartsucker commented Jan 18, 2017

heartsucker commented Jan 25, 2017

conorsch commented Jan 27, 2017

heartsucker commented Dec 3, 2016 •

edited

Loading

redshiftzero commented Dec 7, 2016 •

edited

Loading