fixes #71 - Collaborate on multiple fixes to setup, live-reload, styling. #72

stephenplusplus · 2014-06-18T19:58:51Z

Here's a start!

Changed:

Moved app/styles/sass to app/styles/components
Joined gulp watch and gulp serve into one task: gulp serve

I haven't thoroughly tested yet, but what I can confirm does work:

Edit a app/styles/*.css file, page reloads
Edit a app/styles/components/.scss file, a matching app/styles/components/.css file is created, and the page reloads

stephenplusplus · 2014-06-18T20:37:28Z

Gulp is making my head hurt. But, what I think needs to happen that's not happening now:

Run gulp build
app/styles//*.scss is turned into app/styles//.css and copied to dist/styles/__/.css
All other app/styles//*.css is copied to dist/styles//*.css
autoprefixr is run over dist/styles/*/.css
usemin runs to concat dist/styles/*/.css into minifed file
Extra dist/styles/*/.css files are removed

addyosmani · 2014-06-18T23:42:24Z

Great work, Stephen. It's fantastic to see the live-reload parts working as expected.

I have a quick question: was there a reason we joined gulp watch and gulp serve into a single task? In our gulp generator we still keep them separate. Not asking to revert - just curious. We might want to switch the verbiage to gulp watch as I think that's the terminology we opted for in docs. Will check.

stephenplusplus · 2014-06-18T23:46:26Z

Actually, I thought I was keeping it consistent with generator-gulp-webapp, but I'm clearly mistaken! It's totally ok to revert, but what are the use cases for watching and not serving? Or is it just a matter of modularity that they were separated?

addyosmani · 2014-06-18T23:47:17Z

On your second comment, I understand why you're suggesting we copy the app/styles/*.scss compilations to dist/styles but am missing why the other steps (autoprefixing and so on) need to be changed from how they are done now. It's probably me missing something obvious :)

addyosmani · 2014-06-18T23:48:17Z

Oh, on watch vs serve I believe it was kept separate so that tasks which wanted to call one but not the other had the flexibility to do so. I'd probably revert just that change at the moment but the rest looked good ⭐

sindresorhus · 2014-06-18T23:59:39Z

gulpfile.js

+                precision: 10,
+                loadPath: ['app/styles', 'app/styles/components']
+            })))
+            .pipe($.if(destination === 'dist', $.autoprefixer('last 1 version')))


I actually thought about this exact thing and decided to have it in both livereload and dist as some things will not work without the prefixed property, especially as we support browsersync which works in any browser.

That makes sense. I will run it over css, as I believe it caught a lot of things.

…t css files.

stephenplusplus · 2014-06-19T00:14:22Z

Oh, on watch vs serve I believe it was kept separate so that tasks which wanted to call one but not the other had the flexibility to do so.

So gulp watch will serve? I think maybe what confused me was using gulp watch to trigger the server, where with our other generators, grunt serve would run a server and set up watches, etc. Do you think gulp watch is intuitively-descriptive enough?

sindresorhus · 2014-06-19T00:16:42Z

I was initially confused by this too. I'd prefer if we just had a gulp serve that did both. Users can easily modify if they want something different. But it's a sane default.

addyosmani · 2014-06-19T00:30:08Z

Let's go with gulp serve then.

stephenplusplus · 2014-06-19T00:35:02Z

.tmp folder stuff is in!

addyosmani · 2014-06-19T00:43:29Z

Testing! Thanks for addressing!

stephenplusplus · 2014-06-19T00:43:41Z

I think I asked before how much of a no-no it would be to change from 4 -> 2 spaces, and it was not a no-no. (So, a yes-yes?) Anyway, after this PR is settled, I would love to make the conversion if it's still favored :)

addyosmani · 2014-06-19T00:55:47Z

Hmm. I'm testing the branch you've been working on (ran this 3 times, npm cache clean && reinstalled just to be sure) but am experiencing this issue:

$ gulp
[01:53:09] Using gulpfile ~/projects/google/web-starter-kit/gulpfile.js
[01:53:09] Starting 'clean'...
[01:53:09] Finished 'clean' after 756 μs
[01:53:09] Starting 'default'...
[01:53:09] Starting 'jshint'...
[01:53:09] Starting 'styles'...
[01:53:10] Starting 'html'...
[01:53:10] Starting 'images'...

events.js:74
        throw TypeError('Uncaught, unspecified "error" event.');
              ^
TypeError: Uncaught, unspecified "error" event.
    at TypeError (<anonymous>)
    at Transform.EventEmitter.emit (events.js:74:15)
    at Transform.onerror (/Users/addyo/projects/google/web-starter-kit/node_modules/gulp/node_modules/vinyl-fs/node_modules/through2/node_modules/readable-stream/lib/_stream_readable.js:540:12)
    at Transform.EventEmitter.emit (events.js:95:17)
    at Transform.<anonymous> (/Users/addyo/projects/google/web-starter-kit/node_modules/gulp-useref/index.js:75:42)
    at Array.forEach (native)
    at Transform.<anonymous> (/Users/addyo/projects/google/web-starter-kit/node_modules/gulp-useref/index.js:65:35)
    at Array.forEach (native)
    at Transform.<anonymous> (/Users/addyo/projects/google/web-starter-kit/node_modules/gulp-useref/index.js:43:36)
    at Array.forEach (native)

Note that I haven't run any other gulp commands after checking out the branch, just gulp. Are you seeing something else when you run it?

addyosmani · 2014-06-19T00:57:26Z

And when I run gulp serve:

stephenplusplus · 2014-06-19T00:59:23Z

Strangely, I don't get those at all. Maybe something is confused because of the radically different file structure? My noob suggestion would be cloning in an entirely new directory to see if that magically fixes things.

addyosmani · 2014-06-19T01:06:13Z

Tried again with a new directory and fresh clone but ran into the same problems. @sindresorhus @passy could you verify if you're able to use the branch without these issues?

One other thing I noticed: we should be checking in a pre-built version of components.css in styles so that users not using the tooling can still take advantage. I think this is missing at present. It's okay for the file to be overwritten by the serve process.

sindresorhus · 2014-06-19T01:07:32Z

Yup, getting it too, when you attach an error handler it becomes pretty clear:

gulp-useref: no such file or directory '/Users/sindresorhus/dev/web-starter-kit/{.tmp,app}/styles/components/components.css'

stephenplusplus · 2014-06-19T01:39:35Z

The styles task needs to finish running before the rest of the build process proceeds. I rigged that up the only way I could figure out how :S

stephenplusplus · 2014-06-19T01:40:32Z

gulpfile.js

@@ -78,26 +104,28 @@ gulp.task('pagespeed', pagespeed.bind(null, {
    strategy: 'mobile'
 }));

-gulp.task('clean', rimraf.bind(null, 'dist'));
+gulp.task('clean', function (cb) {
+    rimraf('dist', rimraf.bind({}, '.tmp', cb));


Yep, this is the only way I could figure out how to remove two things.

bind to null, it's clearer that the contents doesn't matter.

addyosmani · 2014-06-19T09:58:24Z

Current bugs:

Blah.css referenced - dropping
Styleguide not hooked up to the new setup - hooking up. There are a few bugs here
Autoprefixer running over our CSS has broken the responsive layout. I'll try to get that sorted.

This works around the fix introduced for not being able to scroll-y on the menu on phone when many links are included.

Although a worthy addition, we’ll have to drop this Stephen :)

addyosmani · 2014-06-19T11:30:16Z

We're at a little bit of an impasse here so any feedback you have on this (at all) is massively useful.
The fixes applied allow us to do the following (including Stephen's changes):

Edit any Sass in app/styles and live-reload works
Edit any Sass/CSS in app/components and it works
Watch the Styleguide and any other HTML for changes and live-reload works, with paths to resources being in-tact (which wasn't the case for the styleguide before)
Build the project and both your HTML and Styleguide work, including paths to assets.

What I'm crankly in light of all this good:

It feels like the Gulpfile is getting increasingly complex. I wonder if this is going to put off beginners.
I'm not happy that I'm including gulp-replace and replacing the path to components.css with the correct one in the Gulpfile. Why am I doing this? Because our Sass references images at a particular path that works fine for anything at the top-level but fails when referencing the built components styles from /styleguide. Base href doesn't seem to be getting us around that. Using a build comment doesn't get us around it either (try it) because then useref gets confused about where things live (index.html referencing vs. /styleguide/index.html referencing same resources from different absolute paths).

If there's a simper way to solve the above without as much hackery I would love to hear it :)

addyosmani · 2014-06-19T12:20:40Z

Based on discussions in IM, it looks like we're happy to go down this direction. Could we get a few more people testing the very latest to double check it all works before we merge?

I might do a pre-merge tag just to be safe.

addyosmani · 2014-06-19T14:29:42Z

It is with a deep sigh and many prayers that I go forth and merge this monster set of changes. Tested workflows as much as possible.

Let's pleeease not make major changes for the next few hours. Style tweaks are coolsies but lets keep it that until we're tagged <333333 Thank you once again for your heroic efforts all.

fixes #71 - Collaborate on multiple fixes to setup, live-reload, styling, guides.

stephenplusplus · 2014-06-19T14:31:04Z

Yay!

fixes #71 - restructure style task.

5e090e5

sindresorhus reviewed Jun 18, 2014
View reviewed changes

autoprefixr back on for app. it had something to say about our curren…

67ea6e1

…t css files.

use a .tmp folder for sass styles.

bf48c25

Rig up the gulpfile.

e2e0334

stephenplusplus reviewed Jun 19, 2014
View reviewed changes

rename title for styles:components task.

2dbba8c

addyosmani added 4 commits June 19, 2014 11:46

Fix the navigation bar being too short on desktop.

b5f030e

This works around the fix introduced for not being able to scroll-y on the menu on phone when many links are included.

Remove unnecessary debug reference

5f5cc0f

Although a worthy addition, we’ll have to drop this Stephen :)

Inline JS for style-guide. Unlikely to be changed.

19d601e

Fixes for style guide image paths, style guide component linking.

d8d0081

Stephen Sawchuk and others added 3 commits June 19, 2014 07:42

formatty things.

346b812

sync readme gulp watch => gulp serve

f22738e

Comment-up the Gulpfile

367f2a2

addyosmani mentioned this pull request Jun 19, 2014

Styleguide JS not picked up in build process #68

Closed

addyosmani added 3 commits June 19, 2014 13:38

Remove alt-layout - basic and index are sufficient

6223cf1

Improvements to the readme

46ad3e4

Further readme improvements

99886fc

addyosmani changed the title ~~fixes #71 - restructure style task.~~ fixes #71 - Collaborate on multiple fixes to setup, live-reload, styling. Jun 19, 2014

addyosmani and others added 8 commits June 19, 2014 14:06

Expand on UnCSS comments

0ab6aa4

Add note on how to opt out of tooling

7f7350f

Add support for toggling snippets - fixes #42

951c1fe

Minor changes to toggling

0cecead

Minor formatting tweak

0003ab3

Drop capturing in the styleguide JS

a6aaee4

ALARM!ALARM! rogue space.

fe8fba6

Further revisions to the readme

45ad936

addyosmani added a commit that referenced this pull request Jun 19, 2014

Merge pull request #72 from google/71-styles

74908b8

fixes #71 - Collaborate on multiple fixes to setup, live-reload, styling, guides.

addyosmani merged commit 74908b8 into master Jun 19, 2014

addyosmani deleted the 71-styles branch June 19, 2014 14:29

espeed mentioned this pull request Jun 21, 2014

Feedback for: /tools/build/build_site.html google/WebFundamentals#498

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fixes #71 - Collaborate on multiple fixes to setup, live-reload, styling. #72

fixes #71 - Collaborate on multiple fixes to setup, live-reload, styling. #72

stephenplusplus commented Jun 18, 2014

stephenplusplus commented Jun 18, 2014

addyosmani commented Jun 18, 2014

stephenplusplus commented Jun 18, 2014

addyosmani commented Jun 18, 2014

addyosmani commented Jun 18, 2014

sindresorhus Jun 18, 2014

stephenplusplus Jun 19, 2014

stephenplusplus commented Jun 19, 2014

sindresorhus commented Jun 19, 2014

addyosmani commented Jun 19, 2014

stephenplusplus commented Jun 19, 2014

addyosmani commented Jun 19, 2014

stephenplusplus commented Jun 19, 2014

addyosmani commented Jun 19, 2014

addyosmani commented Jun 19, 2014

stephenplusplus commented Jun 19, 2014

addyosmani commented Jun 19, 2014

sindresorhus commented Jun 19, 2014

stephenplusplus commented Jun 19, 2014

stephenplusplus Jun 19, 2014

sindresorhus Jun 19, 2014

addyosmani commented Jun 19, 2014

addyosmani commented Jun 19, 2014

addyosmani commented Jun 19, 2014

addyosmani commented Jun 19, 2014

stephenplusplus commented Jun 19, 2014

fixes #71 - Collaborate on multiple fixes to setup, live-reload, styling. #72

fixes #71 - Collaborate on multiple fixes to setup, live-reload, styling. #72

Conversation

stephenplusplus commented Jun 18, 2014

stephenplusplus commented Jun 18, 2014

addyosmani commented Jun 18, 2014

stephenplusplus commented Jun 18, 2014

addyosmani commented Jun 18, 2014

addyosmani commented Jun 18, 2014

sindresorhus Jun 18, 2014

Choose a reason for hiding this comment

stephenplusplus Jun 19, 2014

Choose a reason for hiding this comment

stephenplusplus commented Jun 19, 2014

sindresorhus commented Jun 19, 2014

addyosmani commented Jun 19, 2014

stephenplusplus commented Jun 19, 2014

addyosmani commented Jun 19, 2014

stephenplusplus commented Jun 19, 2014

addyosmani commented Jun 19, 2014

addyosmani commented Jun 19, 2014

stephenplusplus commented Jun 19, 2014

addyosmani commented Jun 19, 2014

sindresorhus commented Jun 19, 2014

stephenplusplus commented Jun 19, 2014

stephenplusplus Jun 19, 2014

Choose a reason for hiding this comment

sindresorhus Jun 19, 2014

Choose a reason for hiding this comment

addyosmani commented Jun 19, 2014

addyosmani commented Jun 19, 2014

addyosmani commented Jun 19, 2014

addyosmani commented Jun 19, 2014

stephenplusplus commented Jun 19, 2014