Basic Environment Setup #8
Conversation
| "important": true, | ||
| "outline-none": false, | ||
| "overqualified-elements": false, | ||
| "text-indent": false |
There was a problem hiding this comment.
The indentation in this file is wrong. Use tabs :)
There was a problem hiding this comment.
The indentation looks wrong in all files! Though I had soft tabs turned off! fixing it now!!
|
Might want to setup some JavaScript code style linting as well? Maybe JSCS? Let's not forget a Do we also want to setup CI (using Travis) from the beginning? Seems like a good idea (even if it's just for linting) |
|
We certainly need another linting. Some of these mistakes should have been pointed out by linting test. I'll add JSCS |
| @@ -0,0 +1,10 @@ | |||
| var express = require( "express" ), | |||
| app, server, config; | |||
There was a problem hiding this comment.
Use separate var for each of these.
| Themeroller:port=5000 | ||
| Themeroller:db_name=themeroller | ||
| Themeroller:db_user=root | ||
| Themeroller:db_password= |
There was a problem hiding this comment.
This file shouldn't be here , I'l delete in next push
|
Looks good to me right now, @arschmitz ? |
| @@ -0,0 +1,8 @@ | |||
| language: node_js | |||
| node_js: | |||
| - "0.10" | |||
There was a problem hiding this comment.
Should we test only on 0.10? We do run 0.10 in production, but testing in 0.12 also is going to save us pain when our production servers update.
There was a problem hiding this comment.
I'll bring this up and all other issues in meeting today one by one, and then update the PR.
|
|
||
| app.use ( "/", express.static( "./static" ) ); | ||
| app.listen( port, function() { | ||
| console.log( "Listening on http://localhost:" + port ); |
There was a problem hiding this comment.
Yes, that's fine - its just a convenience when testing locally. If it turns out later I'm wrong about it, we can still fix it quickly enough...
geekman-rohit
force-pushed
the
setup
branch
2 times, most recently
from
cd80f92
to
f59f234
Compare
|
This PR has been updated. Please do a final review, and if everyone is satisfied, I'll pull it in. |
| <html lang="en"> | ||
| <head> | ||
| <meta charset="utf-8"> | ||
| <title>Themeroller</title> |
There was a problem hiding this comment.
This is picky, but maybe say Chassis Themeroller for the title?
That's really my only feedback I have left on this. Tweak that and I'll pull this in.
There was a problem hiding this comment.
Why did you pull this in already? That's a totally valid point and there's no hurry.
There was a problem hiding this comment.
Umm @arthurvr , me and srisk had a conversation about that on IRC, I needed this environment PR for splitting things into two PRs UI and API and laying a base, that page will soon get replaced by UI page in next PR
There was a problem hiding this comment.
That's no reason to land this already, you could just base those PRs on this branch and rebase it afterwards.
There was a problem hiding this comment.
@arthurvr, it was a minor thing, and since the page was going to be replaced, I decided it was okay if we just did the PR.
Basic Environment Setup. Still working on better way to do the tests, but some reviews would help.