Skip to content

Conversation

sobafuchs
Copy link
Contributor

The script tag in get-started/index.md is incorrectly placed. If someone tries to execute the code to demonstrate how browsers load global variables, they're going to get an error that the browser can't read appendChild of undefined because document.body won't exist yet.

The script tag should be placed at the bottom of the body so that they don't get confused (although this is a pretty inconsequential thing and the whole point of this part of the tutorial is to tell people not to code this way lol).

The script tag in `get-started/index.md` is incorrectly placed. If someone tries to execute the code to demonstrate how browsers load global variables, they're going to get an error that the browser can't read `appendChild` of undefined because `document.body` won't exist yet. 

The script tag should be placed at the bottom of the body so that they don't get confused (although this is a pretty inconsequential thing and the whole point of this part of the tutorial is to tell people not to code this way lol).
@jsf-clabot
Copy link

jsf-clabot commented Nov 17, 2016

CLA assistant check
All committers have signed the CLA.

Since `index.js` is supposed to live inside `app`, it might be confusing to just tell users to create `index.html` without prefixing it with `app/`. Otherwise, the path to `index.js` in the html file will get a 404 back.
@sobafuchs sobafuchs changed the title hotfix/update-script-tag-location Getting Started Clarifications Nov 17, 2016
Since the example seems to continue assuming that `index.html` lives in the root folder, I modified the edit frmo the last commit to instead change how `index.html` accesses `index.js`
@SpaceK33z
Copy link
Member

SpaceK33z commented Nov 17, 2016

There is a second time the wrong <script> src is mentioned, could you also fix that? It's in the diff example.

@sobafuchs
Copy link
Contributor Author

@SpaceK33z should be good to go

@SpaceK33z SpaceK33z merged commit c407056 into webpack:develop Nov 21, 2016
@SpaceK33z
Copy link
Member

Thanks!

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.

4 participants