Skip to content
This repository has been archived by the owner on Nov 16, 2023. It is now read-only.

📝 Readme small updates and fixes #9

Merged
merged 1 commit into from Feb 28, 2018

Conversation

peterblazejewicz
Copy link
Contributor

  • block codes
  • white space unification
  • headers are now grouped by level
  • minor typos

Thanks!

README.md Outdated

First off, open terminal and `cd` to the `TicTacToe_JS` folder. Install dependencies in `package.json`.

```
```js
Copy link
Member

Choose a reason for hiding this comment

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

This isn't js code. 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thx!

README.md Outdated

Before we dive into TypeScript adoption, let's take a look at the structure of the TicTacToe app. It contains a few components and looks like below with or without TypeScript.

```html
Copy link
Member

Choose a reason for hiding this comment

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

This isn't supposed to be a code block. It's supposed to actually be html to render a centered image in the resulting page.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thx!

README.md Outdated
```
node ./node_modules/webpack/bin/webpack.js
```sh
node ./node_modules/.bin/webpack
Copy link
Member

Choose a reason for hiding this comment

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

This is a substance change, but I'd maybe suggest npx webpack instead (this is pretty much npx's intended use, right)? Since this is likely platform-dependent (as the bin folder contains bat files among other things).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wesley, this will expect users to have NPM 5.2.*, well. The intent in the PR was just to shorten the webpack execution path. What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

eh? I don't think you can actually shorten it the way you did then, since node ./node_modules/.bin/webpack doesn't work (on any platform), since the .bin folder contents are shell scripts, not js:
image

And I think we're trying to avoid something complex like "invoke in whatever way is correct for your shell the script at ./node_modules/.bin/webpack", which could be just ./node_modules/.bin/webpack or & ./node_modules/.bin/webpack or a number of other things.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're correct (I've native PC matchine @work only). It works with npx tool. I'll add a line with a link to blog post what the npx tool is
Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, rebased!

- block codes
- white space unification
- headers are now grouped by level
- minor typos
- shortened direct webpack execution with npx tool
@weswigham weswigham merged commit f3a04eb into microsoft:master Feb 28, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants