Js3 Week2 Barbaros #81

whitepiolin · 2018-09-01T10:43:59Z

No description provided.

remarcmij

Hi @whitepiolin, overall pretty good work. Check again the correct order of .catch() and .then() methods in a promise chain (see the link provided in the detail comment).

remarcmij · 2018-09-02T13:19:09Z

homework/src/index.js

+  const repoSelect = createAndAppend("select", divSelect);
+  const divFlex = createAndAppend("section", root);
+  const divInfo = createAndAppend("article", divFlex, { id: "divInfo" });
+  const divCont = createAndAppend("article", divFlex, { id: "divCont" });


Lines 33-44 are best placed inside the main() function, at the beginning, just before the call to fetchJSON(). This will ensure that this code is executed after the web page has been fully loaded. If you place it outside any function like you do here there is a change, for instance, that the div with an id of root hasn't been created in the DOM yet. In that case your code will fail.

I like that you are using semantic HTML tags here instead of just div tag. It reminds me that I should do this more often myself 😉.

remarcmij · 2018-09-02T13:30:23Z

homework/src/index.js

+    const trForks = createAndAppend("tr", tBody);
+    createAndAppend("td", trForks, { html: "Forks : " });
+    const tdForksLink = createAndAppend("td", trForks, );
+    const linkForks = createAndAppend('a', tdForksLink, { html: repository.forks, href: repository.forks_url });


The linkForks variable is unused.

remarcmij · 2018-09-02T13:30:54Z

homework/src/index.js

+    const trRepository = createAndAppend("tr", tBody);
+    createAndAppend("td", trRepository, { html: "Repository : " });
+    const tdRepoLink = createAndAppend("td", trRepository, );
+    const linkRepository = createAndAppend('a', tdRepoLink, { html: repository.name, href: repository.html_url });


The linkRepository variable is unused (you can probably see in VSCode a wavy underline on the variable warning about this). You should only define variables if you have a need for them, otherwise they just become noise.

remarcmij · 2018-09-02T13:32:53Z

homework/src/index.js

+        return;
+      })
+      .then(repositories => {
+        repositories.forEach((repository, index) => {


The assignment asked for the list of repositories to be sorted alphabetically.

remarcmij · 2018-09-02T13:36:49Z

homework/src/index.js

-        createAndAppend('pre', root, { html: JSON.stringify(data, null, 2) });
+  const HYF_REPOS_URL = 'https://api.github.com/orgs/HackYourFuture/repos?per_page=100';
+  const root = document.getElementById('root');
+  createAndAppend("img", root, { src: "./hyf.png", id: "imgLogo" });


This HYF logo is the least important to have in UI but takes up the most prominent place on the web page. It pushes the relevant content down. So, not a JavaScript issue, but more of a UX (User Experience) issue. Make sure that you use screen space (we sometimes call it screen real-estate) in the most effective way.

remarcmij · 2018-09-03T10:32:32Z

homework/src/index.js

+      .catch(error => {
+        const divError = createAndAppend("container", root, { id: "divError" });
+        divError.innerHTML = "";
+        divError.innerHTML = error;


I think @JawharBairakdar already pointed to line 51 serving no purpose in the week 1 review.

remarcmij · 2018-09-03T10:39:58Z

homework/src/index.js

+  function main(HYF_REPOS_URL) {
+
+    fetchJSON(HYF_REPOS_URL)
+      .catch(error => {


The .catch() method should (almost) always be placed as the last method in the promise chain. See the link to the fundamental on promises below. When I force an error by tweaking the url a bit I can see that you are rendering the error in the UI but I also see this error message in the console:

index.js:56 Uncaught (in promise) TypeError: Cannot read property 'forEach' of undefined at fetchJSON.catch.then.repositories (index.js:56)

After the catch() method is executed the JavaScript runtime happily continues executing the next .then() in the chain, which gets the value of undefined for its repositories parameter. That is why you a complaint about trying to use forEach of undefined.

https://github.com/HackYourFuture/fundamentals/blob/master/fundamentals/promises.md#promise-chaining

remarcmij · 2018-09-03T10:40:30Z

homework/src/index.js

+    createAndAppend('h2', divCont, { html: "Contributions", class: "h2" });
+
+    fetchJSON(repository.contributors_url)
+      .catch(error => {


This catch() should go to the end of the chain.

remarcmij · 2018-09-03T10:41:56Z

homework/src/index.js

+    fetchJSON(repository.contributors_url)
+      .catch(error => {
+        createAndAppend('div', root, { html: error.message, class: 'alert-error' });
+        return;


This return is useless. It just return from the arrow function passed to catch(). It would return anyway on the ending curly brace.

remarcmij · 2018-09-03T10:42:33Z

homework/src/index.js

+        const divError = createAndAppend("container", root, { id: "divError" });
+        divError.innerHTML = "";
+        divError.innerHTML = error;
+        return;


This return is useless. It just return from the arrow function passed to catch(). It would return anyway on the ending curly brace.

remarcmij

Looks good! (but still think the logo needs to go or placed elsewhere).

whitepiolin added 2 commits August 30, 2018 19:14

JS3Week1

5509a70

JS3Week2n

bd091f4

profxed added homework2 to review class16 labels Sep 1, 2018

remarcmij added review in progress and removed to review labels Sep 2, 2018

remarcmij previously approved these changes Sep 3, 2018

View reviewed changes

remarcmij added reviewed and removed review in progress labels Sep 3, 2018

whitepiolin added 2 commits September 3, 2018 16:35

Corrections about comments

aee5cd8

some corrections

19a4dd6

whitepiolin dismissed remarcmij’s stale review via 19a4dd6 September 3, 2018 15:08

remarcmij approved these changes Sep 3, 2018

View reviewed changes

remarcmij closed this Oct 14, 2018

Js3 Week2 Barbaros #81

Js3 Week2 Barbaros #81

Uh oh!

Conversation

whitepiolin commented Sep 1, 2018

Uh oh!

remarcmij left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

remarcmij left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants