-
Notifications
You must be signed in to change notification settings - Fork 6.9k
Conversation
…n best practices Partly addresses angular#334.
So there's good news and bad news. 👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there. 😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request. Note to project maintainer: This is a terminal state, meaning the |
55b6a25
to
68df4cf
Compare
Not sure why Travis isn't triggered. I verified this on my fork: https://travis-ci.org/gkalpak/angular-seed/builds/446783519 |
@@ -3,7 +3,7 @@ | |||
This project is an application skeleton for a typical [AngularJS][angularjs] web app. You can use it | |||
to quickly bootstrap your angular webapp projects and dev environment for these projects. | |||
|
|||
The seed contains a sample AngularJS application and is preconfigured to install the Angular | |||
The seed contains a sample AngularJS application and is preconfigured to install the AngularJS |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
README.md
Outdated
Behind the scenes this will also call `bower install`. After that, you should find out that you have | ||
two new folders in your project. | ||
Behind the scenes this will also call `npm run copy-libs`, which copies the AngularJS files and | ||
other frontend dependencies. After that, you should find out that you have two new directories in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
front end is two words (possibly hyphenated).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why rename the folder components
to core
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See comments.
.travis.yml
Outdated
- npm run test-single-run | ||
- (npm start > /dev/null &) && (npm run protractor) | ||
- npm run test-single-run; | ||
- (npm start > /dev/null &) && npm run protractor; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Semicolons are routinely not used in Bash scripts as a new line serves the same purpose and there are no ASI hazards as in JS. I'd leave it as it was.
.travis.yml
Outdated
- npm install | ||
before_install: | ||
- export DISPLAY=":99.0"; | ||
- sh -e /etc/init.d/xvfb start; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this whole block needed now that we can use Chrome Headless?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was a little on the fence on that one. (Might be a little too magic for beginners.)
But I am not opposed. Let's give it a go.
@@ -236,7 +236,7 @@ choose to install the tool globally: | |||
sudo npm install -g http-server | |||
``` | |||
|
|||
Then you can start your own development web server to serve static files from a folder by running: | |||
Then you can start your own development web server to serve static files from any folder by running: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't use "any"! (a bad TypeScript joke)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about unknown
? 😛
@petebacondarwin See #334. |
Includes the following changes: - Run tests on macOS and Windows as well. - Run tests against the current Node.js LTS version (v10.x). - Cache npm cache directory between builds.
2358bb6
to
58a3c46
Compare
@petebacondarwin, @mgol: PTAL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see you are committed to "core"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
Looks like Travis is not enabled for this repo - it must have been disabled some time in the last year: https://travis-ci.org/angular/angular-seed (says "This is not an active repository" for me) |
And we don't have rights to turn it back on... |
Main themes:
Incorporates #361 and #390.
Partially addresses #334.