Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

feat(build): add Jade support #420

Open
wants to merge 3 commits into
from

Conversation

Projects
None yet
Contributor

gonzaloruizdevilla commented Oct 28, 2013

Added new --jade option to generator that will create jade templates (http://jade-lang.com/)

Member

eddiemonge commented Nov 6, 2013

so many changes. @gonzaloruizdevilla can you rebase and merge conflicts please (again i think)(sorry)?

Contributor

gonzaloruizdevilla commented Nov 6, 2013

Done. It was small conflict in app/index.js. In the PR, Bootstrap js dependencies are stored in a list before appending them to the html or jade index file. In the original file, they are passed directly as parameter to the function appendScripts.

Member

eddiemonge commented Nov 6, 2013

can you fix the failing test?

Contributor

gonzaloruizdevilla commented Nov 6, 2013

Please note that the build is failing because my fork project is "generator-angularexpress" and is the express suffix that is responsible for the failure. Mocha tests in my local machine and run in a folder named "generator-angular" pass without problem.

From TravisCi log:

generator-angular@0.5.1 test /home/travis/build/gonzaloruizdevilla/generator-angularexpress
mocha

That folder is responsible of the failing test.

Contributor

gonzaloruizdevilla commented Nov 6, 2013

I have added another commit to the PR. I have modified de .travis.yml file to ensure that the working folder has the same name as the generator. This way the generator can be correctly tested, independently of the repo name.

@passy passy commented on an outdated diff Nov 7, 2013

};
Generator.prototype.createIndexHtml = function createIndexHtml() {
- this.write(path.join(this.appPath, 'index.html'), this.indexFile);
+ if(this.env.options.jade) {
+ this.write(path.join(this.appPath, 'index.jade'), this.indexFile);
@passy

passy Nov 7, 2013

Member

Could you just alter the file name? Something like var indexFile = 'index.' + this.env.options.jade ? 'jade' : 'html';

@passy passy commented on an outdated diff Nov 7, 2013

@@ -269,3 +315,74 @@ Generator.prototype.packageFiles = function () {
this.template('../../templates/common/_package.json', 'package.json');
this.template('../../templates/common/Gruntfile.js', 'Gruntfile.js');
};
+
+Generator.prototype.addMainView = function addMainView() {
+ if(this.env.options.jade) {
+ this.copy('../../templates/common/main.jade', 'app/views/main.jade');
@passy

passy Nov 7, 2013

Member

Same here.

@passy passy commented on an outdated diff Nov 7, 2013

+
+Generator.prototype.addMainView = function addMainView() {
+ if(this.env.options.jade) {
+ this.copy('../../templates/common/main.jade', 'app/views/main.jade');
+ } else {
+ this.copy('../../templates/common/main.html', 'app/views/main.html');
+ }
+};
+
+function appendScriptsJade(jade, optimizedPath, sourceFileList, attrs) {
+ return appendFilesToJade(jade, 'js', optimizedPath, sourceFileList, attrs);
+};
+
+function spacePrefix(jade, block){
+ var prefix;
+ jade.split("\n").forEach( function (line) { if( line.indexOf(block)> -1 ) {
@passy

passy Nov 7, 2013

Member

Single quotes please.

@passy

passy Nov 7, 2013

Member

And jshint white option.

@eddiemonge eddiemonge commented on the diff Nov 7, 2013

app/index.js
@@ -45,6 +45,19 @@ var Generator = module.exports = function Generator(args, options) {
this.env.options.coffee = this.options.coffee;
}
+ if (typeof this.env.options.jade === 'undefined') {
@gonzaloruizdevilla

gonzaloruizdevilla Nov 7, 2013

Contributor

I think this is working right now. I tried:
yo angular --jade
and then
yo angular:view miview
which created a jade file:
create app/views/miview.jade
and
yo angular:view miview2 --jade=false
with created an html file:
create app/views/miview2.html

Anyway, I'm going to add a description for the Jade option, so it is correctly displayed when typing yo angular -help

@eddiemonge

eddiemonge Nov 7, 2013

Member

if that works then something else is broken, such as the auto create jade files if one already exists.

@gonzaloruizdevilla

gonzaloruizdevilla Nov 7, 2013

Contributor

Can you explain a little bit more? When jade files exist, yo is informing that they are identical or that there is a conflict and asking how to solve it.

@eddiemonge

eddiemonge Nov 7, 2013

Member

yo angular:view new --jade should create a new new.jade file
yo angular:view another should create another.jade file (automatically creating the jade file since there are already jade files in the project)
yo angular:view last --jade=false should create last.html since jade false is specified, even though are already jade files

if you dont pass in the jade option then (lines)[https://github.com/gonzaloruizdevilla/generator-angularexpress/blob/72e994c9d4e63dea0f4e6a7d5d78e06724601e3f/app/index.js#L53-L54] should automatically create jade files. passing in false should also do that since this.env.options.jade is undefined

@gonzaloruizdevilla

gonzaloruizdevilla Nov 7, 2013

Contributor

It is working that way. I have tried this:

$ yo angular
....creating proyect
$ yo angular:view myview1
   create app/views/myview1.html
$ yo angular:view myview2 --jade
   create app/views/myview2.jade
$ yo angular:view myview3
   create app/views/myview3.jade
$ yo angular:view myview4 --jade=false
   create app/views/myview4.html
Contributor

gonzaloruizdevilla commented Nov 7, 2013

I've added @passy recommendations and also jshinted the files.
I've added a new commit 48eaaa2 to the PR that fixes a small issue with the tests of AngularJs services.

Member

eddiemonge commented Nov 13, 2013

can you pull the travis fix out and submit that as a new pr and then merge/rebase please?

@eddiemonge eddiemonge commented on the diff Nov 13, 2013

app/index.js
};
+function spacePrefix(jade, block){
@eddiemonge

eddiemonge Nov 13, 2013

Member

I feel like all these functions should go in the utils file

@eddiemonge eddiemonge commented on the diff Nov 13, 2013

app/index.js
@@ -205,16 +281,28 @@ Generator.prototype.bootstrapFiles = function bootstrapFiles() {
return 'styles/' + file.replace('.scss', '.css');
}),
searchPath: '.tmp'
- });
+ };
+
+ if (this.env.options.jade) {
@eddiemonge

eddiemonge Nov 13, 2013

Member
var appendFiles = this.env.options.jade ? appendFilesToJade : this.appendFiles;
appendFiles(filesToAppend)

Would something like that work better so you dont have this block repeated so many times?

@eddiemonge eddiemonge commented on the diff Nov 13, 2013

app/index.js
}
};
Generator.prototype.appJs = function appJs() {
- this.indexFile = this.appendFiles({
- html: this.indexFile,
- fileType: 'js',
- optimizedPath: 'scripts/scripts.js',
- sourceFileList: ['scripts/app.js', 'scripts/controllers/main.js'],
- searchPath: ['.tmp', 'app']
- });
+ if (this.env.options.jade) {
+ this.indexFile = appendFilesToJade({
@eddiemonge

eddiemonge Nov 13, 2013

Member

config block should be a variable and passed into the functions so its not duplicated (at least it looks duplicated to me)

@eddiemonge eddiemonge commented on the diff Nov 13, 2013

templates/common/Gruntfile.js
@@ -30,6 +30,10 @@ module.exports = function (grunt) {
files: ['<%%= yeoman.app %>/styles/{,*/}*.{scss,sass}'],
tasks: ['compass:server', 'autoprefixer']
},<% } %>
+ jade: {
@eddiemonge

eddiemonge Nov 13, 2013

Member

need conditionals for all these. I know there arent really a lot currently in the file for other things but im hoping that will change soon

@gonzaloruizdevilla

gonzaloruizdevilla Nov 13, 2013

Contributor

I thought about it. In my first try, there was a question when running yo angular and the answer was used for the conditionals inside the gruntfile template. However, now it's using an argument for jade templates, so this would mean that the first time this argument is used the Gruntfile must be modified directly, somehow duplicating the conditional logic inside the template.

The same happens with coffescript. If conditionals are used for the coffescript tasks, do you have any ideas of how to modify the gruntfile in a clean way when in a later moment the user decides to use coffescript?

@eddiemonge eddiemonge commented on the diff Nov 13, 2013

templates/common/_package.json
@@ -25,6 +25,7 @@
"grunt-google-cdn": "~0.2.0",
"grunt-ngmin": "~0.0.2",
"time-grunt": "~0.1.0",
+ "grunt-jade": "~0.4.0",
@eddiemonge

eddiemonge Nov 13, 2013

Member

why grunt-jade instead of grunt-contrib-jade?

@gonzaloruizdevilla

gonzaloruizdevilla Nov 13, 2013

Contributor

I've been using grunt-jade for a long time and it has worked fine. I'm going to check grunt-contrib-jade and verify that a few issues I had a long ago with grunt-jade are not present.

@eddiemonge eddiemonge commented on the diff Nov 13, 2013

templates/common/index.jade
+ title
+ meta(name="description", content="")
+ meta(name="viewport", content="width=device-width")
+ // Place favicon.ico and apple-touch-icon.png in the root directory
+ //- build:head
+ body(ng-app="<%= _.camelize(appname) %>App")
+ //[if lt IE 7]>
+ <p class="browsehappy">You are using an <strong>outdated</strong> browser. Please <a href="http://browsehappy.com/">upgrade your browser</a> to improve your experience.</p>
+ <![endif]
+ //[if lt IE 9]>
+ <script src="bower_components/es5-shim/es5-shim.js"></script>
+ <script src="bower_components/json3/lib/json3.min.js"></script>
+ <![endif]
+
+ // Add your site or application content here
+ div(class="container", ng-view)
@eddiemonge

eddiemonge Nov 13, 2013

Member

.container(ng-view)

@gonzaloruizdevilla

gonzaloruizdevilla Nov 13, 2013

Contributor

haha, good catch! :)

@eddiemonge eddiemonge commented on the diff Nov 13, 2013

templates/common/index.jade
+!!!
+// [if lt IE 7]> <html class="no-js lt-ie9 lt-ie8 lt-ie7"> <![endif]
+// [if IE 7]> <html class="no-js lt-ie9 lt-ie8"> <![endif]
+// [if IE 8]> <html class="no-js lt-ie9"> <![endif]
+// [if gt IE 8]><!
+html.no-js
+ //<![endif]
+ head
+ meta(charset="utf-8")
+ meta(http-equiv="X-UA-Compatible", content="IE=edge,chrome=1")
+ title
+ meta(name="description", content="")
+ meta(name="viewport", content="width=device-width")
+ // Place favicon.ico and apple-touch-icon.png in the root directory
+ //- build:head
+ body(ng-app="<%= _.camelize(appname) %>App")
@eddiemonge

eddiemonge Nov 13, 2013

Member

this needs to be updated

Member

eddiemonge commented Nov 13, 2013

why were the view files moved?

Contributor

gonzaloruizdevilla commented Nov 13, 2013

I concluded that files inside templates/common/root are copied as they are, without modifications, and the files that are manipulated like index.html are stored inside templates/common. Maybe this was a wrong assumption?

Member

eddiemonge commented Nov 13, 2013

no i feel like you are right. what i did in my test branch (https://github.com/eddiemonge/generator-angular/tree/jade_support/templates) was create a new folder to hold these so they didnt clutter up other folders

szimek commented Nov 15, 2013

Does it put all generated HTML files in yeoman.app directory? Wouldn't .tmp be better?

Contributor

gonzaloruizdevilla commented Nov 15, 2013

@szimek I agree with your suggestion, I'm going look at it.

szimek commented Nov 15, 2013

@gonzaloruizdevilla If you do change destination path to .tmp, remember to add '.tmp/**/*.html' to livereload.files config as well.

Member

eddiemonge commented Nov 20, 2013

@szimek livereload should still look at the app/*/.jade but the tasks would be jade. it shouldnt watch the tmp folder for html files

szimek commented Nov 22, 2013

@eddiemonge I'm a bit lost :) Will livereload work correctly with jade files having the following config:

watch: {
  jade: {
    files: ['<%= yeoman.app %>/views/**/*.jade'],
    tasks: ['jade:server']
  },
  livereload: {
    options: {
      livereload: '<%= connect.options.livereload %>'
    },
    files: ['lots of other stuff', '<%= yeoman.app %>/views/**/*.jade']
  }
}

Will jade:server task always be executed before livereload reloads the page?

Member

eddiemonge commented Nov 22, 2013

it will if you remove the jade files from livereload. the watch command triggers it automatically. the livereload target is a bit misleading as its really a bucket to catch everything else

szimek commented Nov 22, 2013

So if livereload target is set, it automatically turns livereload for all other targets and additionally for any files listed in its files option?

Member

eddiemonge commented Nov 22, 2013

Sorry you need to set livereload to true in that target

szimek commented Nov 22, 2013

Got it. Thanks!

Hey guys, awesome work here. Is there any chance this will make it into a future release?

dagumak commented Nov 29, 2013

Great work guys! I'm excited to use this!

Member

eddiemonge commented Dec 14, 2013

Im scared of this one but Im still meaning to get to it

This may be a far-future goal, but might it be a good idea to simplify the build process a bit so that stuff like this, and other small custom tasks, can be inserted by users? It'd be neat if there was an easy way to customize angular generator with my own initialization questions like "Would you like to use Jade?".

@eddiemonge eddiemonge referenced this pull request Jan 20, 2014

Open

Generator Roadmap #553

4 of 19 tasks complete
Member

eddiemonge commented Mar 27, 2014

@zakdances those are currently hidden behind a flag.

I do want to land this PR or one like it but got to get to other things first. This is a bug one keeping me from using this generator in my own work so it is a high personal priority but not as high on the project priority. That seems odd, I should change that.

Owner

sindresorhus commented May 10, 2014

ping @eddiemonge

also needs to fix merge conflict.

@eddiemonge eddiemonge added this to the 0.10.0: The Future milestone Jun 5, 2014

@eddiemonge eddiemonge added the on-hold label Jun 5, 2014

Is this still a thing? I'd love to see this in my work.

Member

eddiemonge commented Jul 24, 2015

this needs all the comments addressed, conflicts resolved, commits squashed. It might be better to do it as a new PR with all these changes.

@eddiemonge eddiemonge modified the milestone: 0.10.0: The Future Jul 24, 2015

@eddiemonge eddiemonge added enhancement and removed on-hold labels Jul 24, 2015

Contributor

gonzaloruizdevilla commented Jul 24, 2015

I'll try to find some time to work in a new PR. I'll close this one when the new one is ready.

ricick commented Jan 18, 2016

Is there any update on the status of this PR?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment