refactor(styles): make all of the styles modular #2172

johngruen · 2015-03-03T16:16:43Z

This is still WIP.

Right now, showing the basic, file structure for our Sass going forward. I still need to go through the styles and unpacking things a bit.

@pdehaan: for some reason the breakpoint mixin we've used on Chronicle is throwing an error here. Could you help me diagnose?

coveralls · 2015-03-03T16:30:41Z

Coverage remained the same at 96.17% when pulling 8996f1a on style-wars into 5f6c9a2 on master.

pdehaan · 2015-03-03T18:25:00Z

@johngruen I can help diagnose. Initial theory is that it's a grunt-sass version issue. It seems like fxa-content-server is using grunt-sass@0.14.0 and Chronicle is using grunt-sass@0.18.0. I'll grab this branch and play around locally and run it through scss-lint.

pdehaan · 2015-03-03T18:39:35Z

app/styles/_base.scss

+
+body {
+  color: $text-color;
+  @include font();


Move the include() above the color declaration?

pdehaan · 2015-03-03T18:41:09Z

app/styles/_base.scss

+  h1,
+  h2,
+  h3 {
+    @include headerFont();


_app/styles/base.scss:43 [W] NameFormat: Name of mixin headerFont should be written in all lowercase letters with hyphens instead of underscores

Meh. Not sure if you really want to follow all the scss-lint suggestions, or if we maybe want to disable some of them in the aoo/styles/.scss-lint.yml file.

@pdehaan: stop linting! I haven't done this yet, and it's gonna make you go insane

Yeah, actually the linting isn't that bad. Mostly "Avoid using id selectors.", which I feel like we should turn off since we're all grown-ups. Same with "Avoid qualifying class selectors with an element."

Most of the rest of the lint noise was from the app/styles/_media_queries.scss file which hasn't been touched.

pdehaan · 2015-03-03T18:43:04Z

app/styles/_mixins.scss

+  font-family: $header-font;
+}
+
+@mixin formElement() {


Not sure if these mixins should be header-font() and form-element() to be consistent w/ hidpi-background-image().

pdehaan · 2015-03-03T18:46:50Z

app/styles/_modules.scss

+@import 'modules/cropper';
+@import 'modules/graphic';
+@import 'modules/marketing';
+


If these aren't in a specific order, we could auto-gen this file using something like grunt-sass-globbing (if that's marginally easier to maintain).

Should be some fairly trivial Grunt (and I can do it in a future PR). We'd just have to set up the "sass_globbing" task to generate this "app/styles/_modules.scss" file, and then add the generated file to the .gitignore file, and then run the "sass_globbing" task before the main "sass" task.

pdehaan · 2015-03-03T18:47:35Z

app/styles/main.scss

+//this file still needs to go
+// will be replaced by _breakpoints.scss
+@import 'media_queries'; 
+


The aforementioned sass_globbing trick wouldn't work here since I think these are in a very specific order.

pdehaan · 2015-03-03T18:58:06Z

@johngruen On the topic of errors, where were you seeing the Sass error?
I tried running $ grunt css and I get the following barf:

Running "connect_fonts" task
Warning: Cannot call method 'filter' of undefined Use --force to continue.

Aborted due to warnings.

Running just the "sass" task (via $ grunt sass) seemed to work though:

$ grunt sass
Running "sass:styles" (sass) task
File app/styles/system-font-main.css created.
File app/styles/sync.css created.
File app/styles/main.css created.

Done, without errors.

pdehaan · 2015-03-03T19:00:17Z

app/styles/_variables.scss

@@ -1,5 +1,5 @@
 //Font Family Variables
-$default-font: "Clear Sans", "Helvetica Neue", Helvetica, Arial, sans-serif;
+$default-font:"Fira Sans", Helvetica, Arial, sans-serif;


Interesting! Does this change anything significant by switching from "Clear Sans" to "Fira Sans"?

@pdehaan: Ryan and were talking about nuking Clear Sans, just wanted to see what it would look like. this is probably not going to survive the final version of this PR

Might I suggest something like Comic Neue, for increased whimsy and playfulness. /cc @ryanfeeley

pdehaan · 2015-03-03T19:03:39Z

app/styles/_state.scss

+
+.pulse {
+  animation-duration: $medium-transition;
+  animation-name: pulse;


Any preferences toward using the separate animation-duration and animation-name styles versus the shorthand notation of animation like in .shake, .fadein, etc.?

@pdehaan: i will wind up shortening, i just wanted to get everything moved into new homes before reworking the individual bits.

pdehaan · 2015-03-03T19:05:57Z

app/styles/_state.scss

+  margin-top: 10px;
+}
+
+/* Animation triggering classes */


I'm not 100% on your naming structures, or if it's worth abstracting all these glorious animations into a separate module like _modules/animation.scss to keep them separated but together.

@pdehaan: not a bad idea

pdehaan · 2015-03-03T19:10:02Z

app/styles/modules/_input-row.scss

+  input::-moz-placeholder,  /* Firefox 19+ */
+  input:-ms-input-placeholder {
+    color: $input-placeholder-color;
+  }


Interesting... Doesn't Autoprefixer do this magic for us? Or are we explicitly specifying 18- rules because Autoprefixer doesn't generate prefixes for that far back into the past?

@pdehaan: not sure. probablhy worth tracking down @shane-tomlinson to answer that for us. These were in the Sass before i moved stuff around. Maybe it has to do with 18 being ESR?

johngruen · 2015-03-03T19:10:09Z

@pdehaan

you see the error if you uncomment line 3 in app/styles/main.scss and run grunt server

looks like this:

Running "sass:styles" (sass) task
Warning: /Users/jgruen/Repos/FXA_WEB_SERVER/fxa-content-server/app/styles/breakpoints:4: error: error reading values after small
 Use --force to continue.

Aborted due to warnings.

pdehaan · 2015-03-03T19:10:29Z

app/styles/modules/_input-row.scss

+
+  input[type='email'],
+  input[type='text'] {
+  }


What up with the empty rule, bro?

dude, this has been SHIPPING believe it or not check out https://github.com/mozilla/fxa-content-server/blob/master/app/styles/_general.scss#L382

STOP SHIP! STOP SHIP!

johngruen · 2015-03-03T19:12:17Z

@pdehaan also, why do we use grunt-sass over grunt-contrib-sass?

vladikoff · 2015-03-03T19:17:39Z

also, why do we use grunt-sass over grunt-contrib-sass?

"This task uses libsass which is a Sass compiler in C++."

libsass is faster

pdehaan · 2015-03-03T19:17:43Z

also, why do we use grunt-sass over grunt-contrib-sass?

GLAD YOU ASKED!!!

The grunt-contrib-sass package "requires that you have Ruby and Sass installed" (per https://github.com/gruntjs/grunt-contrib-sass#sass-task). Sometimes managing Ruby and Sass and making sure we're all on the same version can be gross and makes me sad in my space where my heart would be.

The grunt-sass package is "super awesome"(tm) and uses libsass (which is a Sass compiler in C++). It's allegedly faster than the Ruby compiler, but doesn't have as many features, blah blah blah.

johngruen · 2015-03-03T19:23:11Z

@vladikoff @pdehaan cool

johngruen · 2015-03-03T19:24:25Z

also @pdehaan updated grunt-sass to 0.18.0, killed node_modules and reinstalled. Getting the same error :(

pdehaan · 2015-03-03T19:28:12Z

@johngruen I think something got messed up, cause it works for me.

Before:

$ grunt sass
Running "sass:styles" (sass) task
Warning: /Users/pdehaan/dev/fxa-content-server_pd/fxa-content-server/app/styles/breakpoints:4: error: error reading values after small
 Use --force to continue.

Aborted due to warnings.

After:

$ grunt sass
Running "sass:styles" (sass) task

Done, without errors.

You may want to confirm your grunt-sass version number in the package.json file, and you may also need to nuke the npm-shrinkwrap.json file, since that usually tries to enforce which versions get installed.
Try looking in ./node_modules/grunt-sass/package.json for what version is actually installed locally.

pdehaan · 2015-03-05T23:45:53Z

app/styles/_breakpoints.scss

+    @warn 'Unfortunately, no value could be retrieved from `#{$breakpoint}`. '
+    + 'Please make sure it is defined in `$breakpoints` map.';
+  }
+}


I like the organization of breakpoints, but does the @mixin make more sense here, or in _./app/styles/mixins.scss?

I guess similarly, should we leave the breakpoints here in this file, or just in the _./app/styles/variables.scss file?

johngruen · 2015-03-05T23:57:43Z

@pdehaan: WRT your last comment, i think its kind of a wash. I did it as a standalone because (a) it seems kind of special somehow and (b) it seemed nice to define the breakpoint object (or whatever that thing is called in sass-land) with the mixin itself.

FWIW, here's how the modules are going to look:

#fox-logo {

  @include hidpi-background-image('firefox', 80px 85px);
  background-position: center top;
  background-repeat: no-repeat;
  opacity: 0;
  position: relative;
  z-index: $fox-logo-zindex;

  @include respond-to('small') {
    background-size: 48px 51px;
    height: 51px;
    margin: 10px auto 0 auto;
    top: 0;
    width: 100%;
  }

  @include respond-to('big') {
    height: 85px;
    margin: 0 auto;
    top: 30px;
    width: 80px;
  }

  &.static {
    opacity: 1;
  }

  // IE < 10 does not support CSS transforms
  .lt-ie10 & {
    opacity: 1;
  }
}

#about-mozilla {

  @include respond-to('small') {
    display: none;
  }

  @include respond-to('big') {
    @include hidpi-background-image('mozilla', 69px 19px);
    cursor: pointer;
    height: 19px;
    opacity: 0.5;
    position: absolute;
    right: 12px;
    top: 12px;
    transition: opacity $short-transition;
    width: 69px;

    &:hover {
      opacity: 1;
    }

    &:focus {
      outline-color: $html-background-color;
    }
  }
}

pdehaan · 2015-03-06T00:17:31Z

That's some pretty Sass! Let me know if there is anything else I can help with or review.

#xoxo

pdehaan · 2015-03-06T21:58:47Z

Weird, I'm currently seeing this error when I try and run % grunt sass locally:

$ grunt sass

Loading "sass.js" tasks...ERROR
>> TypeError: Object #<Object> has no method 'info'
Warning: Task "sass" not found. Use --force to continue.

Aborted due to warnings.

$ npm -v; node -v

1.4.28
v0.10.35

UPDATE: Looks like Travis is choking as well for some reason: https://travis-ci.org/mozilla/fxa-content-server/builds/53400026#L532

$ node --version
v0.10.36

$ npm --version
1.4.28

$ nvm --version
0.23.3

UPDATE^2: Looks like if I re-re-delete my ./node_modules/ directory and blow away the npm-shrinkwrap file, then it installs and runs fine:

$ grunt sass
Running "sass:styles" (sass) task

Done, without errors.

pdehaan · 2015-03-19T16:21:42Z

@johngruen Is this still WIP, or is it ready for a rebase+merge?

pdehaan · 2015-03-19T18:11:18Z

OK, I spent an hour manually going through a bunch of flows and clicking links and everything on https://ux.dev.lcip.org/looks fantastic. Great work @johngruen! 👍 from me.

ckarlof · 2015-03-19T19:33:17Z

Thanks @pdehaan!

Looks like it needs a rebase, which is mildly concerning about whether we need a further look-see or not, depending on whether the rebase touches anything touched by this PR.

@johngruen what do you think?

johngruen · 2015-03-19T19:36:57Z

@ckarlof @pdehaan: looks like the conflict is in npm-shrinkwrap.js. not sure how to proceed here.

vladikoff · 2015-03-19T19:44:44Z

@johngruen , maybe delete your npm-shrinkwrap update, amend commit, rebase with master, update shrinkwrap, amend commit, update pr?

pdehaan · 2015-03-19T19:50:02Z

@johngruen The good news is that the style-wars branch is in this repo, so if you're truly at a loss, you can punt and get @shane-tomlinson or @vladikoff to resolve the shrinkwrap 💩.

coveralls · 2015-03-20T15:01:31Z

Coverage increased (+0.11%) to 97.18% when pulling 5f2e9a1 on style-wars into f063516 on master.

johngruen · 2015-03-20T20:58:46Z

Hmmm, @vladikoff followed your advice and now i'm failing...could use an assist getting this one to turn green!

coveralls · 2015-03-23T14:29:29Z

Coverage remained the same at 97.07% when pulling 529f27f on style-wars into f063516 on master.

johngruen · 2015-03-23T14:45:08Z

@vladikoff @pdehaan I re-ran npm shrinkwrap without the --dev flag and I'm now passing! Is this cool?

vladikoff · 2015-03-23T15:18:03Z

@johngruen Yeap!

zaach · 2015-03-23T17:16:27Z

package.json

@@ -48,7 +48,7 @@
    "grunt-po2json": "git://github.com/shane-tomlinson/grunt-po2json.git#c1a7406",
    "grunt-requirejs": "0.4.2",
    "grunt-rev": "0.1.0",
-    "grunt-sass": "0.14.0",
+    "grunt-sass": "^0.18.1",


Let's use the exact version (no ^).

coveralls · 2015-03-23T17:52:11Z

Coverage increased (+0.11%) to 97.18% when pulling d4b30fd on style-wars into f063516 on master.

shane-tomlinson · 2015-03-23T19:16:41Z

app/styles/modules/_password-row.scss

+}
+
+/**
+ * IE8 blows up when changing the type of the password field


Do we need this anymore since we no longer support IE8?

@shane-tomlinson: probably not, but there's some test code in app/scripts/vendor/env-test.js that goes along with these styles. Maybe we do a separate bug to back out ie legacy styles and tests separately?

@johngruen - sounds good, mind filing an issue to remove IE8 support from CSS except for the "browsehappy" related stuff?

WHOA 2+2+2+2 = 8

shane-tomlinson · 2015-03-23T19:19:21Z

app/styles/modules/_password-row.scss

+}
+
+/* this one is impossible to do w/o sibling selector
+we may need to add js for IE8*/


We no longer care about IE8, so this comment is irrelevant.

shane-tomlinson · 2015-03-23T19:28:02Z

@johngruen - the "/confirm_account_unlock" screen needs to be updated:

johngruen · 2015-03-23T19:29:43Z

@shane-tomlinson /complete_account_unlock as well...pushing now

coveralls · 2015-03-23T20:29:22Z

Coverage increased (+0.11%) to 97.18% when pulling cb4651f on style-wars into f063516 on master.

zaach · 2015-03-23T20:47:42Z

👍

shane-tomlinson · 2015-03-23T20:49:02Z

r+ from me.

@zaach

refactor(styles): make all of the styles modular r=@zaach, @shane-tomlinson, @pdehaan

johngruen added the waffle:needs review label Mar 3, 2015

pdehaan reviewed Mar 3, 2015
View changes

app/styles/_base.scss

body {

color: $text-color;

@include font();

This comment has been minimized.

Sign in to view

pdehaan Mar 3, 2015
Contributor

Move the include() above the color declaration?

pdehaan reviewed Mar 3, 2015
View changes

app/styles/_mixins.scss

font-family: $header-font;

}

@mixin formElement() {

This comment has been minimized.

Sign in to view

pdehaan Mar 3, 2015
Contributor

Not sure if these mixins should be header-font() and form-element() to be consistent w/ hidpi-background-image().

pdehaan reviewed Mar 3, 2015
View changes

johngruen force-pushed the style-wars branch 2 times, most recently from d21a557 to 9cec07a Mar 3, 2015

pdehaan reviewed Mar 5, 2015
View changes

johngruen force-pushed the style-wars branch from 9cec07a to 097af65 Mar 6, 2015

johngruen changed the title ~~WIP: refactor all the styles~~ refactor(styles): make all of the styles modular Mar 6, 2015

johngruen force-pushed the style-wars branch from 097af65 to 00686f5 Mar 6, 2015

johngruen force-pushed the style-wars branch from 6d6297e to 5f2e9a1 Mar 20, 2015

johngruen force-pushed the style-wars branch from 5f2e9a1 to 529f27f Mar 23, 2015

zaach reviewed Mar 23, 2015
View changes

johngruen force-pushed the style-wars branch from 529f27f to d4b30fd Mar 23, 2015

shane-tomlinson reviewed Mar 23, 2015
View changes

app/styles/modules/_password-row.scss

}

/* this one is impossible to do w/o sibling selector

we may need to add js for IE8*/

This comment has been minimized.

Sign in to view

shane-tomlinson Mar 23, 2015
Member

We no longer care about IE8, so this comment is irrelevant.

refactor(styles): make all of the styles modular

cb4651f

johngruen force-pushed the style-wars branch from d4b30fd to cb4651f Mar 23, 2015

shane-tomlinson pushed a commit that referenced this pull request Mar 23, 2015

Merge pull request #2172 from mozilla/style-wars

f2a2d9c

refactor(styles): make all of the styles modular r=@zaach, @shane-tomlinson, @pdehaan

shane-tomlinson merged commit f2a2d9c into master Mar 23, 2015
2 checks passed

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

continuous-integration/travis-ci/push The Travis CI build passed
Details

shane-tomlinson removed the waffle:needs review label Mar 23, 2015

shane-tomlinson deleted the style-wars branch Mar 23, 2015

johngruen mentioned this pull request Mar 23, 2015

Refreshing on the ToS/PN pages breaks layout/back button #2044

Closed

mozilla / fxa-content-server Archived

refactor(styles): make all of the styles modular #2172

refactor(styles): make all of the styles modular #2172

Conversation

johngruen commented Mar 3, 2015

coveralls commented Mar 3, 2015

pdehaan commented Mar 3, 2015

This comment has been minimized.

pdehaan Mar 3, 2015 Contributor

This comment has been minimized.

pdehaan Mar 3, 2015 Contributor

This comment has been minimized.

johngruen Mar 3, 2015 Author Contributor

This comment has been minimized.

pdehaan Mar 3, 2015 Contributor

This comment has been minimized.

pdehaan Mar 3, 2015 Contributor

This comment has been minimized.

pdehaan Mar 3, 2015 Contributor

This comment has been minimized.

pdehaan Mar 3, 2015 Contributor

pdehaan commented Mar 3, 2015

This comment has been minimized.

pdehaan Mar 3, 2015 Contributor

This comment has been minimized.

johngruen Mar 3, 2015 Author Contributor

This comment has been minimized.

pdehaan Mar 3, 2015 Contributor

This comment has been minimized.

pdehaan Mar 3, 2015 Contributor

This comment has been minimized.

johngruen Mar 3, 2015 Author Contributor

This comment has been minimized.

pdehaan Mar 3, 2015 Contributor

This comment has been minimized.

johngruen Mar 3, 2015 Author Contributor

This comment has been minimized.

pdehaan Mar 3, 2015 Contributor

This comment has been minimized.

johngruen Mar 3, 2015 Author Contributor

johngruen commented Mar 3, 2015

This comment has been minimized.

pdehaan Mar 3, 2015 Contributor

This comment has been minimized.

johngruen Mar 3, 2015 Author Contributor

This comment has been minimized.

pdehaan Mar 3, 2015 Contributor

johngruen commented Mar 3, 2015

vladikoff commented Mar 3, 2015

pdehaan commented Mar 3, 2015

johngruen commented Mar 3, 2015

johngruen commented Mar 3, 2015

pdehaan commented Mar 3, 2015

This comment has been minimized.

pdehaan Mar 5, 2015 Contributor

johngruen commented Mar 5, 2015

pdehaan commented Mar 6, 2015

pdehaan commented Mar 6, 2015

pdehaan commented Mar 19, 2015

pdehaan commented Mar 19, 2015

ckarlof commented Mar 19, 2015

johngruen commented Mar 19, 2015

vladikoff commented Mar 19, 2015

pdehaan commented Mar 19, 2015

coveralls commented Mar 20, 2015

johngruen commented Mar 20, 2015

coveralls commented Mar 23, 2015

johngruen commented Mar 23, 2015

vladikoff commented Mar 23, 2015

This comment has been minimized.

zaach Mar 23, 2015 Contributor

coveralls commented Mar 23, 2015

This comment has been minimized.

shane-tomlinson Mar 23, 2015 Member

This comment has been minimized.

johngruen Mar 23, 2015 Author Contributor

This comment has been minimized.

shane-tomlinson Mar 23, 2015 Member

This comment has been minimized.

johngruen Mar 23, 2015 Author Contributor

pdehaan Mar 3, 2015
Contributor

pdehaan Mar 3, 2015
Contributor

johngruen Mar 3, 2015
Author Contributor

pdehaan Mar 3, 2015
Contributor

pdehaan Mar 3, 2015
Contributor

pdehaan Mar 3, 2015
Contributor

pdehaan Mar 3, 2015
Contributor

pdehaan Mar 3, 2015
Contributor

johngruen Mar 3, 2015
Author Contributor

pdehaan Mar 3, 2015
Contributor

pdehaan Mar 3, 2015
Contributor

johngruen Mar 3, 2015
Author Contributor

pdehaan Mar 3, 2015
Contributor

johngruen Mar 3, 2015
Author Contributor

pdehaan Mar 3, 2015
Contributor

johngruen Mar 3, 2015
Author Contributor

pdehaan Mar 3, 2015
Contributor

johngruen Mar 3, 2015
Author Contributor

pdehaan Mar 3, 2015
Contributor

pdehaan Mar 5, 2015
Contributor

zaach Mar 23, 2015
Contributor

shane-tomlinson Mar 23, 2015
Member

johngruen Mar 23, 2015
Author Contributor

shane-tomlinson Mar 23, 2015
Member

johngruen Mar 23, 2015
Author Contributor

johngruen Mar 23, 2015
Author Contributor

shane-tomlinson Mar 23, 2015
Member