Feedback homework week 3

[remarcmij]

Hi Isaac, here is my feedback on the final week 3 homework.

This review is based on your OOPClasses folder.

Overall I'm very pleased with what I see in terms of functionality and code quality. Your application works fine and is responsive, although the layout get a bit screwed up on certain device types, for instance on the iPhone 6/7/8 Plus.

1. It would be nice if you could click on a contributor to open a new Tab with that contributor's GitHub page. That would also make it a bit more of a challenge to make your application ARIA-compliant (so that a user can also navigate through the contributor list using the keyboard).

With respect to the JavaScript code:

2. Your code looks well organised. I don't see any ESLint warnings and only one or two spelling warnings because you chose to use Contr as abbreviation for Contributors. I still think your code is now and then unnecessarily stretched out vertically due to an abundance of unneeded blank lines. This makes it for instance difficult to oversee in one glance the whole of your function start() in App.js. For that function I would probably condense some of your code and use blank lines to separate blocks of related code like this:

async start() {
  try {
    const url = 'https://api.github.com/orgs/HackYourFuture/repos?per_page=100';
    const root = document.getElementById('root');

    this.createAndAppend('h1', root, {
      html: 'HYF SPA <br> BY <br> >> OOP ES6 Classes <<'
    });

    const header = this.createAndAppend('div', root, { id: 'header' });
    this.createAndAppend('label', header, { html: 'Select a Repository:' });

    const container = this.createAndAppend('div', root, {
      class: 'container',
      id: 'container'
    });

    const informationDiv = this.createAndAppend('div', container, { class: 'infoDiv' });
    this.createAndAppend('ul', informationDiv, { id: 'info' });

    const imagesDiv = this.createAndAppend('div', container, { class: 'imgDiv' });
    this.createAndAppend('ul', imagesDiv, {  id: 'imgUl' });

    const data = await this.fetchJSON(url);
    this.manipulateSelect(data);
  } catch (err) {
    document.getElementById('container').innerHTML = err.message;
  }
}

I recommend that you use blank lines only for a reason. Remove any blank line for which you cannot come up with a good reason for why it's there.

3. You have duplicated code: fetchJSON() in App.js and fetchContributors() in Repository.js do exactly the same thing. You could just add fetchJSON() to the View base class, making it available to all its subclasses. Note that you don't need the async keyword for fetchContributors(). You only need that keyword if in the function body you use await.

4. Your render() function in the Repository class doesn't use await and therefore doesn't need to be prefixed with the async keyword. There is also no need for try/catch in that function because there is no exception to be expected here. Although the async and try/catch obviously do not prevent your code from running correctly, they are confusing to the reviewer of your code (thinking: am I missing something here??).

5. I intended the Contributor class to hold one contributor only. That's why I chose the word Contributor over Contributors. However, your implementation, where you use the Contributor class for the array of contributors is not wrong per se.

6. Your function sortList() in View.js is now dead code (i.e. unused) and should be removed.

With this, I have no doubt you will do well in the rest of the curriculum.

[gisaac85]

Thank you very much Jim. Really I am happy with your reviews and feedback. I will deal with them today. And I wish that i do well in the rest of the curriculum. I am really very happy in HYF.

[gisaac85]

Hoi Jim, i have changed all what you mentioned in issue OOP week3 . I hope it is good now 👍

[remarcmij]

Hi Isaac, thanks for making the changes. I have one final comment:

The Repository class has all the data it needs to completely manage itself (in OOP terms this is called data encapsulation). Therefore its fetchContributors() method doesn't need an external caller to tell which URL to use by supplying it as its argument. Instead, the method could look like this:

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

[gisaac85]

👍 👍 👍