Added to the scope JS 101 article#201
Conversation
There was a problem hiding this comment.
Two things:
- suggest "it can be accessed from anywhere within a program" rather than "it can be got at from anywhere"
- this paragraph ends as though the thought will be continued after the code example, but it is not continued
|
@rmurphey thanks for your feedback, I guess that teaches me for doing the pull request in a bit of a rush! Based on your feedback I have reworded and reworked things. As for if the attribution is out of date, I am unsure. I presumed the contents of the file before I edited were from jQF, and I've not removed it. |
There was a problem hiding this comment.
Better:
JavaScript also creates a Local Scope inside each function body. For exampmle:
|
On reading this content in greater depth, I think it would be good to spend some more time integrating it into the existing content -- presently it seems like there's some duplication of content between the old and new content, and it seems like some of the examples at the end of the section are no longer necessary, or should have some prose explanation associated with them. I can try to take a look at this tomorrow, but I'm not sure how best to submit the changes -- as a pull request on @jackfranklin's repo, or as a pull request to this repo that simply builds upon what's here. @ajpiano? I've added a couple more comments inline. I'd also suggest:
|
|
Thanks for your feedback @rmurphey :) I will have a crack at integrating the two bits more closely, and bring the file up to date with the style guide. I've not got any immediate idea as to why it's not merge-able, unless the Original MD file has changed since I pulled. I'll have an explore as to why. |
|
Just did some more tweaking. I've no idea why Github says this isn't merge-able though! I made a start on integrating the two "bits" of the article and adding some prose around the larger code examples. Not perfect by any means but a start. |
|
@jackfranklin Try pulling from upstream master and merging into the branch, or rebasing the branch on the current master. I know I did a lot of changes in the formatting/example code in this article. |
|
@gnarf37 thanks, I did a pull from upstream and indeed there was a conflict in scopes.md which I fixed. This does mean that this pull request is now awfully messy though with that latest commit that fixed the conflicts. Would it be best to delete this one, and I'll re-fork, apply my changes and do a new pull request so there's a lot less noise? |
|
@jackfranklin the diff doesn't seem that noisy - You could start over if you think that will result in a better pull though, I won't stop you :) |
|
If you're happy I'll leave it :-) I will make those adjustments tomorrow. |
There was a problem hiding this comment.
This seems like a restatement of the first reason not to have global variables. I'm hesitant to just say a thing is a bad practice without saying why it's a bad practice, but in fact you've already said why it's a bad practice, so this paragraph seems redundant to me.
I'd simply point out that IIFEs provide a way to avoid polluting the global scope -- ensuring that variables can't be tampered with by other code -- and move on to the example below.
There was a problem hiding this comment.
@rmurphey do you mean to remove the entire jQuery IIFE example, or just reword the line that starts "Secondly..." to mention IIFEs, then show the jQuery example, and then move onto the next example (which starts on line 59) ?
Following on from #89 (comment), I finally found some time to quickly move over some of the content I wrote for my initial Scope and this in JS article and move it over into the Scopes article, reworking it slightly but not making any major changes.
Let me know what you think :)