Week1 #95

kawaraa · 2018-10-14T18:30:06Z

Here is my homework for week1

Finishes homework week1

Adds styles.

remarcmij

Hi @kawaraa, your application works so I will approve your PR. But I think you made some unfortunate design decisions which makes your code rather cumbersome to maintain. Please take good note of the comments below.

remarcmij · 2018-10-23T19:00:02Z

homework/src/index.js

    xhr.responseType = 'json';
    xhr.onload = () => {
-      if (xhr.status < 400) {
+      if (xhr.status === 200) {


By changing the condition of the if statement you are now considering any status code not equal to 200 as an error. This is an error by itself. Any code equal or greater than 400 is an error. All codes less than 400 are not errors. See: https://www.restapitutorial.com/httpstatuscodes.html

If you expect a 200 code but get some other code but still less than 400 you need to find some other way of dealing with. But you can't report it to the user as an HTTP error.

This still needs to be fixed.

remarcmij · 2018-10-23T19:01:31Z

homework/src/index.js

        cb(null, xhr.response);
      } else {
-        cb(new Error(`Network error: ${xhr.status} - ${xhr.statusText}`));
+        cb({ text: `Error: ${xhr.status} - ${xhr.statusText}`, id: "id", val: "error" });


By convention, error information is reported through a standard JavaScript Error object (as the original code did). Why did you choose to change this?

remarcmij · 2018-10-23T19:01:59Z

homework/src/index.js

    };
-    xhr.onerror = () => cb(new Error('Network request failed'));
+    xhr.onerror = () => {
+      cb({ text: "Network request failed", id: "id", val: "error" });


Use an Error object (as per the original code).

remarcmij · 2018-10-23T19:05:05Z

homework/src/index.js

  }

-  function createAndAppend(name, parent, options = {}) {
+  function createEl(name, parent, obj) {


Your createEl() function is a less capable version than the original createAndAppend() function. The original createAndAppend() could set any attribute. Your version can only deal with the id attribute. I have a hard time understanding why you chose to dumbed down this function.

remarcmij · 2018-10-23T19:06:28Z

homework/src/index.js

+      const header = createEl("header", root);
+      createEl("h3", header, { text: "HYF Repositories" });
+      const select = createEl("select", header);
+      let names = [];


ESLint warns that you could have used const instead of let. You should follow up on such warnings.

remarcmij · 2018-10-23T19:38:37Z

homework/src/index.js

-      }
-    });
+    if (obj) {
+      if (obj.text) { elem.innerHTML = obj.text; }


The original function used innerText instead of innerHTML. We did that on purpose, because we want to discourage this use of innerHTML for creating HTML elements.

remarcmij · 2018-10-23T19:39:42Z

homework/src/index.js

+    let a = createEl("a", td, { text: details.name, id: "href", val: details.html_url });
+    a.setAttribute("target", "_blank");
+    if (details.description) {
+      createEl("tr", table, { text: `<th>Description:</th><td>${details.description}</td>` });


As mentioned, we discourage the use of innerHTML for creating HTML elements.

remarcmij · 2018-10-23T19:41:27Z

homework/src/index.js

+    createEl("th", tr, { text: "Repository:" });
+    let td = createEl("td", tr);
+    let a = createEl("a", td, { text: details.name, id: "href", val: details.html_url });
+    a.setAttribute("target", "_blank");


The original createAndAppend() function could have set this target attribute.

remarcmij · 2018-10-23T19:42:18Z

homework/src/index.js

+    let tr = createEl("tr", table);
+    createEl("th", tr, { text: "Repository:" });
+    let td = createEl("td", tr);
+    let a = createEl("a", td, { text: details.name, id: "href", val: details.html_url });


Your createEl() function doesn't know how to deal with a val property (what would be its purpose anyway)?

remarcmij · 2018-10-23T19:43:22Z

homework/src/index.js

+    fetchJSON(url, (err, info) => {
+      if (err === null) {
+        let ul = createEl("ul", parent);
+        for (let i = 0; i < info.length; i++) {


Why not a .forEach()?

remarcmij · 2018-11-08T10:50:35Z

homework/src/index.js

@@ -1,47 +1,103 @@
-'use strict';
+"use strick";


This is a silly typing error to make (and flagged by the spelling checker). Your code is now not using strict mode.

I can see all you have mentioned and I will fix them and come back again! thanks for your comments

kawaraa · 2018-11-09T15:28:31Z

I'm done with all requirements, and I did my best to make it better and readable.
Again thanks for your time our teacher!

remarcmij

Hi @kawaraa, you still haven't addressed the important comment to not include all code in one gigantic main() function.

remarcmij · 2018-11-10T21:58:03Z

homework/src/index.js

    xhr.responseType = 'json';
    xhr.onload = () => {
-      if (xhr.status < 400) {
+      if (xhr.status === 200) {


This still needs to be fixed.

remarcmij · 2018-11-10T22:00:47Z

homework/src/index.js

-      if (key === 'text') {
-        elem.innerText = value;
+    for (let key in options) {
+      if (key === "txt") {


There is really no reason to use cryptic abbreviations. Why not just used text instead of txt or name instead of nm. Professionals write clean code, see for instance this link:

https://medium.com/mindorks/how-to-write-clean-code-lessons-learnt-from-the-clean-code-robert-c-martin-9ffc7aef870c

remarcmij · 2018-11-10T22:01:36Z

homework/src/index.js

-'use strict';
+"use strick";
+
+function main(url) {


This is still not fixed.

kawaraa added 2 commits October 14, 2018 20:24

Update index.js

f03d2fe

Finishes homework week1

Update style.css

035281f

Adds styles.

remarcmij added homework1 to review class17 labels Oct 14, 2018

remarcmij added review in progress and removed to review labels Oct 23, 2018

remarcmij previously approved these changes Oct 23, 2018

View reviewed changes

remarcmij added reviewed and removed review in progress labels Oct 23, 2018

Implements naming convention.

958e7bf

kawaraa dismissed remarcmij’s stale review via 958e7bf November 7, 2018 20:47

remarcmij reviewed Nov 8, 2018

View reviewed changes

Ahmad Kawara added 2 commits November 8, 2018 20:27

fixes the bugs that Jim refered to.

d3e5e0a

Change status condition.

2905677

remarcmij suggested changes Nov 10, 2018

View reviewed changes

Update index.js

27ec181

remarcmij closed this Nov 29, 2018

Week1 #95

Week1 #95

Uh oh!

Conversation

kawaraa commented Oct 14, 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!

kawaraa commented Nov 9, 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants