Week1

[mehmetscoskun]

No description provided.

[remarcmij]

Hi Mehmet, some minor comments on the use of functions, but overall I'm pretty satisfied with what you produced here. I'll approve this PR for week 1. Try and address my comments in your week 2 PR.

[remarcmij]

While the starter code used the generic name data to describe the data returned by the call to fetchJSON(), we actually know that the data consists of an array of objects about repositories. The code becomes more readable if we change the name data to repositories. This is a general principle: choose names that best describe what the variable holds.

[remarcmij]

There is no need to use getElementById to access this div. As you can see from inspecting the source code for the createAndAppend() function, it returns the element that is created. So you can replace lines 39 and 41 by a single line:

const myHeading = createAndAppend('div', root, { id: 'myheading' });

[remarcmij]

Similar to the previous comment, you can combine lines 45 and 47. Moreover, since you do not use selectId for CSS styling, you can then also leave out that id.

[remarcmij]

Assuming that you renamed data to repositories as I suggested earlier, you could make this forEach a bit more clear:

repositories.forEach(repository => createAndAppend('option', select, { html: repository.name, class: 'rep-names' }); 

Later in your code you raised the issue about the most effective way to find out the index of the selected repository. Well, you can explicitly set the value attribute of each option element to the index of the array, like this:

repositories.forEach((repository, index)  => {
  createAndAppend('option', select, {
     value: index,
     html: repository.name, 
     class: 'rep-names' 
  });
});

Then, in your event listener you can pick up the value attribute (which now is an index value) to pick up the corresponding repository object:

const selectedRepository = repositories[select.value];

Note that is preferable to use 'dot' notation to access properties rather than using the bracket notation: obj['name'] vs obj.name. The only case where you must use brackets is when the actual property name comes from some other variable (i.e. a 'computed' property name), for instance:

for (const propName in repository) {
   console.log(propName + '=' + repository[propName]);
}

[remarcmij]

If you want use a constructor function then there are some conventions that you are supposed to follow.

1. The constructor function must be named to reflect the object you want to create. It must start with an upper case letter (the only time that a function is not camelCase named). It should not have a verb in its name. For instance:

  function RepositoryDetail(repo) {
    this.description = repo.description;
    this.forks = repo.forks;
    const someDate = new Date(repo.updated_at);
    this.updated = someDate.toLocaleString();
  }

Property names should be in camelCase (start with a lowercase letter).

Also, you should define this function at the file level, not inside another function.

[remarcmij]

Combine lines 80 and 82 as explained before. (I won't repeat this comment should there be any further occurrences of this same issue further down in the code.)

[remarcmij]

Combine 92 and 94. (I won't repeat this any more should there be further occcerances

[remarcmij]

You should not normally define functions inside other functions, except when you dealing with relative short functions that you pass as a callback or event handler to some other function.

You can define showDetails() at the file level, but you will need to pass the repository information that it needs to show as parameter to that function, e.g.:

function showDetails(selectedRepo) {
  //...
}

[remarcmij]

So, the overall structure of your file could be:

'use strict';
{
  function fetchJSON(...) {
    //...
  }

  function createAndAppend(...) {
    //...
  }

  function main() {
    //...
   showDetails(repositories[0]);
   //...
   select.addEventListener('change', function () {
      //...
      showDetails(repositories[select.value]);
    });
  }
 
  function showDetails(selectedRepo) {
    //...
  }

  function RepositoryDetails(repo) {
    //...
  }

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

  window.onload = () => main(HYF_REPOS_URL);
}

[mehmetscoskun]

thank you @remarcmij , your comments show me my mistakes and weak points I must improve. Besides, @JawharBairakdar explained a few points I could not understand well. I make some changes according to your comments and the short tutorial given by Jawhar :

first of all, I deleted 'getElementbyId' lines and combined the newly defined 'const' variables with the 'createAndAppend' functions.

secondly, I tried to give better and more explanatory names to the variables and the functions. for example, I used 'data' as a parameter of functions and 'repositories' and 'repository' as an argument.

thirdly and the most important, I took the functions in the 'main' function outside of the 'main' function. I defined three functions outside of the 'main' function and called them back inside of the 'main' function. I did not create a new file for the new functions. I hope what I did is true.

fourthly I create 'contributions' box. And then I made some changes in 'style.css' file. My page is now responsive but I added only one media query. I suppose it works well.

Thanks to you and Jawhar my codes look more legible and understandable :)

You can see all I did on week2 homework.

See you next Sunday