Skip to content

Restructure the Solution for 'Army of Functions' task and Fix Typos #2081

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

Merged
merged 2 commits into from
Sep 10, 2020

Conversation

MuhammedZakir
Copy link
Contributor

@MuhammedZakir MuhammedZakir commented Aug 25, 2020

Fix #2068 - Army of Functions
Fix #2070 - Typo
Fix #2056 - Grammatical Error
Fix #2074 - Remove semi-colon after function declaration

@CLAassistant
Copy link

CLAassistant commented Aug 25, 2020

CLA assistant check
All committers have signed the CLA.

@iliakan
Copy link
Member

iliakan commented Aug 27, 2020

Anything wrong with the SVG?

@MuhammedZakir
Copy link
Contributor Author

MuhammedZakir commented Sep 1, 2020

Encountered some problem on my end. Everything fixed now!

Anything wrong with the SVG?

I fixed it by replacing the SVG with a new one!

@MuhammedZakir MuhammedZakir marked this pull request as draft September 1, 2020 10:23
@MuhammedZakir MuhammedZakir marked this pull request as ready for review September 1, 2020 11:51
@MuhammedZakir MuhammedZakir force-pushed the master branch 3 times, most recently from 07a22fc to 89e3c77 Compare September 2, 2020 06:16
Copy link
Member

@iliakan iliakan left a comment

Choose a reason for hiding this comment

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

  1. Please see the comments.
  2. Split PR please. One PR covers a lot. This way I can accept it partially.


```js run demo
function makeArmy() {
As you can see above, on each iteration of a `while {...} ` block, a new lexical environment is created. This implies that as long as we store the value of `i` in a variable in the `while {...}` block, created Lexical Environment will have that variable with value of `i`.
Copy link
Member

Choose a reason for hiding this comment

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

The second phrase is hard to understand.

Copy link
Contributor Author

@MuhammedZakir MuhammedZakir Sep 4, 2020

Choose a reason for hiding this comment

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

created Lexical Environment will have that variable with value of i.

^This?

Copy link
Member

Choose a reason for hiding this comment

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

This implies that as long as we store the value of i in a variable in the while {...} block, created Lexical Environment will have that variable with value of i.

Copy link
Contributor Author

@MuhammedZakir MuhammedZakir Sep 10, 2020

Choose a reason for hiding this comment

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

Does the entire sentence not making any sense or is it the way it is phrased? Do you have any idea how to simplify it? I couldn't think of anything atm. :-(

P.S. I think people can grasp the meaning if it is read along with the snippet below.

@iliakan
Copy link
Member

iliakan commented Sep 3, 2020

Please make different PRs. This one is quite big for review, I keep postponing it.

Fix javascript-tutorial#2068 - Army of Functions
Fix javascript-tutorial#2070 - Typo
Fix javascript-tutorial#2056 - Grammatical Error
Fix javascript-tutorial#2074 - Remove semi-colon after function declaration
@MuhammedZakir
Copy link
Contributor Author

Please make different PRs. This one is quite big for review, I keep postponing it.

I have removed them.

@iliakan
Copy link
Member

iliakan commented Sep 10, 2020

Could you make the requested minor changes please?

@MuhammedZakir
Copy link
Contributor Author

Could you make the requested minor changes please?

Done.

@iliakan
Copy link
Member

iliakan commented Sep 10, 2020

I'll merge it and review the part about the task.

PLEASE make separate PRs for different articles =) instead of 1 huge PR (multiple branches help).

@iliakan
Copy link
Member

iliakan commented Sep 10, 2020

Thanks!

@iliakan iliakan merged commit 0168147 into javascript-tutorial:master Sep 10, 2020
@iliakan
Copy link
Member

iliakan commented Sep 10, 2020

@MuhammedZakir I thoroughly re-examined and further updated http://javascript.info/task/make-army, basing on your suggested changes, hope it's even better now.

@MuhammedZakir
Copy link
Contributor Author

MuhammedZakir commented Sep 10, 2020

PLEASE make separate PRs for different articles =) instead of 1 huge PR (multiple branches help).

Sorry about that! It won't happen again! :-)

@MuhammedZakir I thoroughly re-examined and further updated http://javascript.info/task/make-army, basing on your suggested changes, hope it's even better now.

Looks good! 👍 I found some grammar mistakes. As I am a bit busy now, I will make a PR later.

Thanks!

You are welcome! :-)

Edit: Website is not updated. Are you perhaps updating it manually and not automatically on each new commit?

MuhammedZakir added a commit to MuhammedZakir/en.javascript.info that referenced this pull request Sep 18, 2020
MuhammedZakir added a commit to MuhammedZakir/en.javascript.info that referenced this pull request Sep 18, 2020
Fix grammar and indent some paragraphs.
Complements javascript-tutorial#2081.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants