-
Notifications
You must be signed in to change notification settings - Fork 6.9k
chore(*): refactor, improve, upgrade #444
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 |
| 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).
petebacondarwin
left a comment
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?
mgol
left a comment
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.
| ``` | ||
|
|
||
| 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 |
petebacondarwin
left a comment
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"
mgol
left a comment
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.