muratkaymaz56/hyf_javascript3

hyf_javascript3's Issues

Feedback homework week 1

Hi Murat,

Here is my feedback on your week 1 homework.

My comments are minor. Overall you have done an excellent job. Your application works fine and your code looks organized.

1. Your repo is missing both a .ignore and a .eslintrc file. The .ignore file is used to exclude certain files from being tracked by Git. For instance, you have a file .DS_Store in your repo which has nothing to do with the project. You are probably working on a Mac because that is a Mac-specific file. I asked in class to create an .eslintrc file in your repo. Without that file you are missing out on the useful help and warnings that ESLint can give you. Please create an .eslintrc file in the root of your hyf-javascript3 folder and paste in the contents that I posted in slack during class. In your case, when I add that file I only get one warning using var in line 130 (use const or let) and a missing semicolon in line 144.

2. When I start your application I see the "AngularJS" entry in your <select> element, but the corresponding information is not displayed. This is because you fetch and render data when there is a 'change' event from the <select> element, but such an event is only triggered when the user changes the selection. You could call your event handler manually at application startup, but because your onSelectChanged() expects an event object that is not so easy. However, with a couple of changes will can make that work.

First, clever to store the repository information as a JSON string in the value of each <option> element. However, there is a simpler solution. We can directly use the information (an array of repository information) we get back from the first call fetchJSON() as shown here. (By the way, you don't need this API object to define the url, a simple const will do):

const hyfUrl = 'https://api.github.com/orgs/HackYourFuture/repos?per_page=100';

function getRepos() {
    fetchJSON(hyfUrl, (err, repositories) => {
        if (err) {
            console.log(err);
            return;
        }

        const root = document.getElementById('root');
        const header = append('header', root);
        const title = append('span', header);
        title.innerHTML = "HYF Repositories";

        const select = append('select', header);
        select.addEventListener('change', () => onSelectChanged(repositories[select.value]));

        repositories.forEach((repo, index) => {
            const option = append('option', select);
            option.setAttribute('value', index);
            option.innerHTML = repo.name;
        });

        const main = append('main', root);
        main.innerHTML = "<div id='left-sidebar'></div><div id='right-sidebar'></div>";

        onSelectChanged(repositories[0]);
    });
}

Rather than stuffing the repository information as a JSON string in each <option> element, we can just store the array index of each element of the repository information array. The fat arrow function used here as the event handler retains access to the repositories array (remember "closure"?). We can simple pass a single element from the repositories array to your onSelectChanged() event handler. We can also call onSelectChanged() manually with repositories[0] to display the repository information for the first one in the list at application startup.

3. You created many HTML elements by creating the structure as a string and then assigning it to the innerHTML of some parent element. That is a perfectly valid approach.

4. Since your getRepos() function does all the startup and initialization, I would probably rename that to main() and rather than calling it directly would use:

window.onload = main;

5. I would take out some redundant blank lines, e.g. in fetchJSON().

That's it! Well done!

Feedback homework week 3

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

You have successfully converted your app to ES6 classes, using async/await with promises. The distribution of responsibilities across the various classes is exactly right. The code is well-formatted.

With this end result, I am confident that you can be successful throughout the rest of the HYF curriculum.

Having said that, there are still some comments to make.

1. You did not make your app responsive (however, this is a CSS issue).

2. You are not rendering (network) errors to the web page. End-users will not have the console open to scan for errors.

3. You did not sort the repositories by name in the select box.

4. The initial repo in the select box at start-up is not rendered in the web page (you did it correctly in the week 2 homework).

5. There is a problem with your use of window.onload. The way you coded it does not actually cause your code to be executed when the page is fully loaded. Instead it is executed immediately when the JavaScript engine loads the script file, which may be too early. If I move the <script src="app.js"> tag from <body> to <header> (which is a valid thing to do), you app no longer works. I get this error in the console:

app.js:146 Uncaught (in promise) TypeError: Cannot read property 'appendChild' of null
    at createAndAppend (app.js:146)
    at View.initialize (app.js:71)
    at new View (app.js:63)
    at app.js:150

The window.onload property must be assigned a function. That function will be called when the page is completely loaded. For instance, you could do this (using an arrow function):

window.onload = () => new View();

Feedback homework week 2

Hi Murat,

Here is my feedback on your week 2 homework.

I have not much to say. Your code looks good and works well. The callbacks are correctly converted to promises. But most of the work we did together in our hangout, so I'm not surprised.

However, you didn't implement the rendering of (network) errors to the HTML page as I asked in this slack message. Please take care of that in the week 3 homework.