Skip to content
This repository was archived by the owner on May 14, 2024. It is now read-only.

Conversation

@eugeneFr9
Copy link

Finished the application, week2

Copy link
Contributor

@remarcmij remarcmij left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Andrew Evgenii, while there are some issues you need to look at I think you did a good job here.

Unfortunately you did not completely follow the homework instruction and create your homework in the /homework folder. As a consequence your code was not checked by ESLint.

Please address the comments in your week 3 homework.

@@ -0,0 +1,93 @@
'use strict';

const URL_Rep = 'https://api.github.com/orgs/HackYourFuture/repos?per_page=100';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From your indentation I suspect that you intended to create a block scope here to prevent your function from polluting the global scope. However, to do so, you must use curly braces, i.e.:

'use strict';
{
  const URL_Rep = ...
  //...


const URL_Rep = 'https://api.github.com/orgs/HackYourFuture/repos?per_page=100';
function renderRepository(container, repository) {
let dateOfRepository = new Date(repository.updated_at);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because you created your homework in a different folder than was specified in the homework assignment, your code is not checked by ESLint (you are missing an .eslintrc file in the Week1 folder where you placed your homework). These are the warnings and errors that ESLint produced:

/Users/jimcramer/hackyourfuture/class16/genegr88/Week1/github.js
   4:9   warning  Identifier 'URL_Rep' is not in camel case                    camelcase
   6:9   warning  'dateOfRepository' is never reassigned. Use 'const' instead  prefer-const
  13:90  warning  Missing semicolon                                            semi
  59:9   warning  Missing semicolon                                            semi
  69:94  warning  Missing semicolon                                            semi
  72:14  warning  'err' is defined but never used                              no-unused-vars
  73:31  warning  'error' is not defined                                       no-undef
  86:72  warning  Missing semicolon                                            semi
  87:89  warning  Missing semicolon                                            semi
  91:70  warning  Missing semicolon                                            semi
  92:9   warning  Missing semicolon                                            semi
  95:2   error    Newline required at end of file but not found                eol-last

✖ 12 problems (1 error, 11 warnings)

createAndAppend('td', tr3, {text: dateOfRepository.toLocaleString()});


}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The preceding two blank lines are meaningless and should be removed.

Add a single blank line between function definitions.

//contributors box
function renderContributors(containerForContributors, repository) {
containerForContributors.innerHTML = "";
const contributorsURL = repository['contributors_url'];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use dot notation to access object properties if you know the property name beforehand:

const contributorsURL = repository.contributors_url;

This gives IDE's such as VSCode and linters such ESLint a better chance to analyze your code.

Use square brackets only if the property name is "computed", e.g.

const fullName = `${firstName} ${lastName}`;
const person = people[fullName];

}
//contributors box
function renderContributors(containerForContributors, repository) {
containerForContributors.innerHTML = "";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You have a mix of single and double quotes for string delimiters. Choose one and then use it consistently throughout your code.

createAndAppend('div', containerForContributors, { text: err.message, class: 'alert-error' });
})

.then(contributors => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The .catch() should be at the end of the promise chain. Currently your error handling does not work correctly because the .catch() method returns a new promise that resolves to undefined (because you are not returning anything in the function body of your catch handler). Next, you chain a .then on that new promise which expects a contributors array but instead gets undefined. While it might look okay in the UI, the code is wrong.


fetchJSON(url)
.catch(err => {
container.innerHTML = error.message;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. The .catch() should be at the end of the promise chain.
  2. The argument is called err but you are trying to access error.

I assume you haven't actually tried to force an error as the assignment asked, otherwise you would have caught this error.

createAndAppend('td', contributorsTr, { text: contName, class: 'inform-contrib contributor-name' });

createAndAppend('td', contributorsTr, { text: contNum, class: 'inform-contrib contributor-number' });
});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Below is the HTML that your code generates for the contributors table. If you look at the documentation for tbody you will find that the only children allowed at tr elements.

<table id="contributorsTable">
  <tbody id="contributorsTbody">
    <a href="https://github.com/gijscor" target="_blank" class="contributorsLink">
      <tr id="15912395" class="contributorsTr">
        <td id="gijscorTd" class="inform-contrib">
          <img src="https://avatars0.githubusercontent.com/u/15912395?v=4" alt="picture of gijscor" class="contributor-images">
        </td>
        <td class="inform-contrib contributor-name">gijscor</td>
        <td class="inform-contrib contributor-number">12</td>
      </tr>
    </a>
    <a href="https://github.com/wouterkleijn" target="_blank" class="contributorsLink">
      <tr id="37488847" class="contributorsTr">
        <td id="wouterkleijnTd" class="inform-contrib">
          <img src="https://avatars1.githubusercontent.com/u/37488847?v=4" alt="picture of wouterkleijn" class="contributor-images">
        </td>
        <td class="inform-contrib contributor-name">wouterkleijn</td>
        <td class="inform-contrib contributor-number">4</td>
      </tr>
    </a>
  </tbody>
</table>

select.addEventListener('change', (event) => {
renderRepository(container, repositories[event.target.value])
renderContributors(containerForContributors, repositories[event.target.value])
});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You will notice that there is a slight delay between showing the repo info and the contributors info. Time-wise it looks like this.

[show repo][clear contributors container][fetch contributors].......[show contributors]

You can improve on the user experience by postponing the (re-)rendering until the contributor data has been returned:

[fetch contributors].......[clear contributors container][render contributors][show repo]

Now, while the contributors are being fetched the screen remains unchanged.

To achieve this in your code, swap the two lines as follows:

renderContributors(containerForContributors, repositories[event.target.value]);
renderRepository(container, repositories[event.target.value]);

Then, in your renderContributors() function, postpone clearing out the containerForContributors until after you have received the response from the network:

function renderContributors(containerForContributors, repository) {
  const contributorsURL = repository['contributors_url'];
  createAndAppend('p', containerForContributors, { text: 'Contributions' });
  fetchJSON(contributorsURL)
    .then(contributors => {
      containerForContributors.innerHTML = "";
     //...

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants