Week2

[shadoo77]

No description provided.

[remarcmij]

Hi Shadi, good work again this week. Some minor comments on naming. See my comment on the second event listener: something to address in the week 3 homework.

[remarcmij]

Introducing an intermediate variable promise helps to understand here what fetchJSON() returns. This is a good approach when learning. Later you will use promises all over the place and you will probably not bother to create intermediate variables, but chain directly from the function returning a promise, e.g.

fetchJSON(url)
  .then(data => {
    //...
  })
  .catch(...

Since we know that we are getting a list (i.e. array) of repositories, the code would become a bit more readable if you renamed the generic meaningless name data to repositories. Your code could then read as:

repositories.sort((a, b) => a.name.localeCompare(b.name, 'fr', {ignorePunctuation: true}));
repositories.forEach((repository, index) => {
  createAndAppend('option', select, {html: repository.name, value: index});
});

Note that you could also chain this as:

repositories
  .sort((a, b) => a.name.localeCompare(b.name, 'fr', {ignorePunctuation: true}))
  .forEach((repository, index) => {
    createAndAppend('option', select, {html: repository.name, value: index});
  });

[remarcmij]

This comment is from week 1:

You have probably taken the localeCompare arguments from an example in MDN, but there is really reason why should specifically configure it for the French language (argument 'fr').

[remarcmij]

This comment is from week 1:

Use a single blank line between function definitions.

[remarcmij]

I would rename data to repositories. In general, use generic, meaningless names such as data, info etc only when the variable or parameter can contain anything. When you know, as in this case, exactly what the function parameter will receive, choose a names that accurately describes it contents.

[remarcmij]

This is a comment from week 1:

Since you are repeating code here (create a tr with two tds as children, you could create a separate function to avoid repetition (the DRY principle: Don't Repeat Yourself), for instance:

  function (table, label, value) {
    const tr = createAndAppend('tr', table);
    createAndAppend('td', tr, { html: `${label} :`, class: 'label' });
    createAndAppend('td', tr, { html: value });
  }

[remarcmij]

data -> contributors

[remarcmij]

Variable, parameter and function names should be camelCase. Only constructor functions and ES6 classnames should be PascalCase. See the naming convention fundamental.

[remarcmij]

You have added a second event listener here. A single event listener that handles both the repository data (left-hand) and contributor data (right-hand) is generally preferred. This simplifies understanding cause and effect in the app, especially for other developers looking at your code. They may not expect a second handler for the same event.

[remarcmij]

In terms of naming, all these contr prefixes are unnecessary, as in the context of this function everything evolves around contributors.