fadi-rasheed / hyf-javascript3 Goto Github PK

View Code? Open in Web Editor NEW

0.0 0.0 0.0 8 KB

JavaScript 68.38% HTML 4.98% CSS 26.63%

hyf-javascript3's People

Contributors

Watchers

hyf-javascript3's Issues

Feedback homework week 2

Hi Fadi, here is my feedback on your week 2 homework.

I think you picked up most of my comments from the week 1 feedback. The promises are correctly implemented, but see point 1 below.

1. I think you missed point 3 from my week 1 feedback. You could modify your rendersAll() function like this to use the contributors_url property returned from the 'outer' fetchJSON() call, like this:

function rendersAll(selectedRepo) {
  const ur = 'https://api.github.com/repos/HackYourFuture/' + selectedRepo;
  fetchJSON(ur)
    .then(repo => {
      return fetchJSON(repo.contributors_url)
        .then(contributors => {
          renderCb(repo);
          renderContributors(contributors);
        });
    })
    .catch(error => {
      const err = document.getElementById('root');
      err.innerHTML = error.message;
    });
}

You only need a single .catch() statement at the end of the chain, provided you return the promise from the inner fetchJSON() call.

2. At the end of your file you have this statement:

window.onload = main();

This has the same effect as just using this:

main();

Your main() function will be executed as soon as the JavaScript engine encounters this call. Sometimes, it may call it too soon, because the web page and all of its resources is not fully loaded yet. For instance, where you do

const root = document.getElementById('root');

the <div id="root"></div> may not have been loaded yet. The correct way to using window.onload is like this:

window.onload = main;

Notice that I left out the parentheses.

You can experiment to see the difference between window.onload = main(); and window.onload = main; by moving the <script> tag to the header:

<!DOCTYPE html>
<html>
<head>
    <meta charset="utf-8" />
    <meta http-equiv="X-UA-Compatible" content="IE=edge">
    <title>Home Work week 2</title>
    <link rel="stylesheet" href="style.css">
    <script src="app.js"></script> 
</head>
<body>
    <div id="root"></div>
</body>
</html>

You should find your screen to remain blank if you use window.onload = main();. Leaving out the parentheses should fix the problem, because now the browser will wait until the page is fully loaded before calling your main() function.

2. You still have an undefined variable contributors in line 71. Did you see the green underline warning about that generated by ESLint?

3. Your code indentation is off. Do you use VSCode with the right settings as documented in the VSCode Tips?

4. You may want to use the enhanced version of createAndAppend() that we used in class with the Nobel Prize app. It will make your code more concise. See this slack message.

5. When your application starts it showsAngularJS in the select box but no data is shown for this repo. This is because your rendersAll() function is only called when the user changes the selection. But you can "manually" call renderAll() yourself at the end of of renderRepos():

function renderRepos(repos) {
  // ...
  repos.forEach(repo => {
    const listItem = createAndAppend("option", select);
    listItem.innerHTML = repo.name;
    listItem.setAttribute("value", repo.name);
  });

  rendersAll(repos[0].name); // <== added
}

Feedback homework week 1

Hi Fadi, here is my feedback on your week 1 homework.

Overall, your app seems to work fine. It is nicely styled.

Let's look at the code now.

1. Your repo should contain a .gitignore and a .eslintrc file. It may well be that you have them in your working directory but did not push them. See my recent message in slack on how to push commits to GitHub.

2. When I add a .eslintrc file to your repo I get a couple of warnings about undefined variables: info in line 60 and contributors in lines 80 and 85. The fact that your code still works is because of an obscure feature of browsers, maintained for backward compatibility, which causes id attributes in your HTML to become available as global JavaScript variables. It is a bad practice to rely on this obscure behavior. To access a DOM element by id, used the document.getElementById() method, e.g.:

const info = document.getElementById('info');

3. The assignment required you to use the contributors_url property from the repository data obtained from GitHub to access the contributors data:

The response object that is returned by GitHub from the request to get repository information includes a property with the contributors_url. Use the value of this property to make a new request to GitHub to obtain a list of contributors.

Instead you have constructed the url yourself, which is not conforming to the assignment. Using the contributors_url property would make the fetchJSON call in line 104 dependent on the result of the preceding fetchJSON call in line 103. It would allow you to practice how to make nested asynchronous calls. I would encourage you to make this change for the purpose of practicing.

3. To make it very explicit what is happening at startup time, I would place all code that is not inside any other function into a function called main() and let the browser execute that function on the 'load' event:

function main() {
    function callBack(error, data) {
        if (error !== null) {
            console.error(error);
        } else {
            renderRepos(data);
        }
    }

    fetchJSON(url, callBack);
}

window.onload = main;

4. When it comes to naming, please don't use unnecessary abbreviations. For instance, renderContributors is much more clear than renderCon. Or renderRepository instead of renderCb. It is only a few characters more to type. The namerenderRepo will also be fine because it is generally understood that 'repo' is short for 'repository'.

The function renders() is better called renderAll() or something like that, because your function should imply an 'action', and therefore must use some form of verb.

Please assume that you write your code for other people to read (for instance, your colleague who needs to make a change to your code some time next year).

5. Spelling error: 'titel' -> 'title'. Don't ignore spelling errors.

6. Do you use the code formatter in VSCode? Your code is formatted non-standard. For instance, there should be a space after a comma. Fat arrow function definitions should have a space around the arrow, e.g. () => {', etc. I also see some missing semicolons.

As you can see, my comments are minor. Please fix the issues that I have addressed. Apart from those, well done!

Feedback homework week 3

Hi Fadi, here is my feedback on your homework for week 3.

Your application loads and runs OK. Although it appears to be responsive it is unusable on a mobile phone because the font size for the repo info becomes much too small (see image).

Your code looks well organised and well formatted. You have correctly implemented promises and async/await.

1. You didn't implement ES6 classes, but don't worry about it. It will be covered again in the React module, which is still some time away.

2. You did not sort the list repos displayed in the select box as was asked in Step 4 of the homework.

3. You need to fix your ESLint setup and, once done, fix the issues flagged by ESLint, e.g. undeclared variables, unused variables etc.

fadi-rasheed / hyf-javascript3 Goto Github PK

hyf-javascript3's People

Contributors

Watchers

hyf-javascript3's Issues

Feedback homework week 2

Feedback homework week 1

Feedback homework week 3

Recommend Projects

React

Vue.js

Typescript

TensorFlow

Django

Laravel

D3

Recommend Topics

javascript

web

server

Machine learning

Visualization

Game

Recommend Org

Facebook

Microsoft

Google

Alibaba

D3

Tencent