Skip to content
New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove references to Bower #26

Merged

Conversation

BradenLawrence
Copy link
Contributor

 - Partially addressing issue ember-learn#21
 - Followup to PR ember-learn#19
 - Replaced Bower with Yarn where it makes sense
 - Explained interaction between yarn.lock and package-lock.json
 - Explained how EmberCLI does not watch for npm install packages
 - Did NOT address any of the Bower references within code examples
@locks locks requested a review from Turbo87 October 20, 2018 11:39
the dependencies of your project.
Changes to your dependencies should be managed through these files, rather
than manually installing packages individually.
Ember CLI supports [NPM](https://www.npmjs.com) and [yarn](https://yarnpkg.com/)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

small nitpick: npm should be lowercase, while yarn should by Yarn according to their websites 😉

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for pointing this out. I noticed their website uses 'npm' lowercased even in the middle of sentences too. I'll go ahead and change all instances of NPM to npm.

Ember CLI supports [NPM](https://www.npmjs.com) and [yarn](https://yarnpkg.com/)
for dependency management. A newly generated Ember CLI project only has NPM
dependencies, so it will include a `package.json` file and use NPM by default.
If you include a `yarn.lock` file, the CLI will detect it and use Yarn instead.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

include a yarn.lock file

"include" might be somewhat hard to understand 🤔

Copy link
Contributor

@Gaurav0 Gaurav0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. Just one thing...

@@ -49,8 +40,7 @@ root (not the default `ember-cli-build.js`).

To add an asset specify the dependency in your`ember-cli-build.js` before
calling `app.toTree()`. You can only import assets that are within the
`bower_components` or `vendor`
directories. The following example scenarios illustrate how this works.
`vendor` directories. The following example scenarios illustrate how this works.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is out of date. We can now import from node_modules.

 - Change all instances of NPM to npm
 - Change all instances of yarn to Yarn
 - Clarify users would be installing and using Yarn,
   not just including a yarn.lock file
 - Add node_module as a location assets can be imported from
@BradenLawrence
Copy link
Contributor Author

Thanks for the review! I tried to clear up the wording around the yarn.lock file as best I could, but let me know if you have any suggestions.

Correct capitalization and improve clarity
- Change all instances of NPM to npm
- Change all instances of yarn to Yarn
- Clarify users would be installing and using Yarn,
not just including a yarn.lock file
- Add node_module as a location assets can be imported from

@Gaurav0 Gaurav0 merged commit 0b684e2 into ember-learn:master Oct 26, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants