Feedback homework week 3

remarcmij commented 6 years ago

Hi Aya, here is my feedback on your homework for week 3 (OOP version):

When I load your index.html file in the browser it looks good and functions as expected. However, your app is not responsive. On an (emulated) iPhone 6/7/8 it looks like this:

aya

On your code, I have little to comment. As last time it is a pleasure to read. Still, there are some minor improvements possible.

1. The sorting of the repository information could be simplified by using the standard string method .localeCompare():

repos.sort((a, b) => a.name.localeCompare(b.name));

The code that you used obviously works, however it does not take into account local language peculiarities. For instance, suppose you have an array with the following French words (listed in the correct dictionary order):

const frenchWords = [ 'effusion', 'égal', 'emballer' ];

The result from sorting:

[ 'effusion', 'emballer', 'égal' ]  // <= your method
[ 'effusion', 'égal', 'emballer' ]  // <= localCompare

The .localeCompare() method might even be able to sort Arabic words (which I know to have no uppercase letters) in the correct dictionary order (perhaps you can try).

2. In the assignment, it was suggested that you create a method fetchContributors() in the Repository class. Because the Repository class has all the information needed (in your case in this._infoData) to manage itself completely, it can fetch the contributors without an external caller needing to tell it what URL to use. So this fetchContributors() method could look like this:

fetchContributors() {
  return fetchJSON(this._data.contributors_url);
}

However, to make this work you would need to move the fetchJSON() function out of the View class and make it a stand-alone function again, similar to createAndAppend().

With this fetchContributors() method in the Repository class your render() method of the View class could look like this:

async render(repoData) {
  try {
    const ulInfo = document.getElementById('infoUl');
    const ulImg = document.getElementById('imgUl');

    const repository = new Repository(repoData);
    const data = await repository.fetchContributors();
    const contributors = new Contributor(data);
    repository.render(ulInfo);
    contributors.render(ulImg);
  }
  catch (err) {
    renderError(err);
  }
}

Now, your View class can remain completely unaware about the internal details of the repository information (that's a good thing).

3. 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:

Uncaught (in promise) TypeError: Cannot read property 'appendChild' of null
    at createAndAppend (app.js:112)
    at View.mainPage (app.js:54)
    at new View (app.js:47)
    at app.js:149

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();

Overall, you have done very well in JS3 class and I have no doubt you will do well in the rest of the HYF curriculum too.

Aya-Alabrash commented 6 years ago

I fix it still media query remains (make it responsive)

remarcmij commented 6 years ago

You changes look good. I would advise you to use flexbox instead of floats to make the app responsive. Then, it is easy to switch the repo div and the contributor div from flex-direction: row; (large screen) to flex-direction: column (tablet/phone), using a media query.

Aya-Alabrash / hyf-javascript-3

Feedback homework week 3 #3