Conversation
| This time I learned to ask for help sooner than later since I got stuck with fetching pull requests inside a function. | ||
| The problem was solved by nesting the fetch of API for pull requests inside the first fetch. Another problem I encountered was not knowing how to use the tokens correctly and how to make Netlify show my index.html site. But after some added information in the notion page the problem was easily fixed! | ||
| If I had more time I would try to play around with API:s more and presenting the data with another chart. | ||
|
|
There was a problem hiding this comment.
Interesting and detailed description of problem solving.
There was a problem hiding this comment.
Thank you so much for all your inputs, Josse. I am glad you found the comments to the code easy to follow too! :)
code/script.js
Outdated
| const options = { | ||
| method: 'GET', | ||
| headers: { | ||
| Authorization: `token ${GITHUB_TOKEN}` | ||
| } | ||
| } |
There was a problem hiding this comment.
Nice work to implement the token and not give up even if obstacles during the "road". For the Authorization part, you don't have to have template literals - I saw that Karin had it in here code also - not really sure why and the reason for having the word "token" before the API const name, do you? Otherwise you've could just have written:
| const options = { | |
| method: 'GET', | |
| headers: { | |
| Authorization: `token ${GITHUB_TOKEN}` | |
| } | |
| } | |
| const options = { | |
| method: 'GET', | |
| headers: { | |
| Authorization: 'GITHUB_TOKEN' | |
| } | |
| } |
Josse79
left a comment
Josse79
left a comment
There was a problem hiding this comment.
Very well done Stephannie. I can see that you are careful when structuring and commenting your code - not leaving a single thing behind. Therefore it has been a pleasure to review your code. Be proud!
| fetch(USER_PROFILE, options) | ||
| .then(res => res.json()) | ||
| .then(data => { | ||
| const user = data.name |
There was a problem hiding this comment.
I think actually that we should have our github user name included (SteppbySteph) - that will be fetched by user.login. It might be due user name is unique in github and therefor easier for anyone to find the correct Stephannie Medenilla. Although in your case there was only one Stephannie Medenilla when I did a search :-) I can see that some of our teammates have both name and user name.
| // Creating constsant for my filtered repos | ||
| forkedRepos = data.filter((repo) => repo.name.startsWith('project-')) | ||
|
|
||
| // Creating variable to pass on to showChart(). | ||
| allMyRepos = forkedRepos.length | ||
|
|
||
| // forEach function to create HTML elements & pull requests | ||
| forkedRepos.forEach((repo) => { | ||
|
|
||
| // Getting Repo Name | ||
| repoName = repo.name; | ||
|
|
||
| // Getting Most Recent Push Date which will be formated according to 'substr' method. | ||
| pushDate = repo.pushed_at.substr(0, 10) | ||
|
|
||
| // Getting the Default Branch | ||
| defaultBranch = repo.default_branch | ||
|
|
||
| // Getting the URL of the repo | ||
| urlRepo = repo.html_url | ||
|
|
||
| // Creating innerHTML to insert above info |
There was a problem hiding this comment.
Really nice commenting as this is difficult parts to get hold of if not explained.
| if(myPR) { | ||
| getCommitNr(myPR, repo.name) | ||
| } else { | ||
| document.getElementById(`commit-${repo.name}`).innerHTML += 'No pull request made' | ||
| } |
| const getCommitNr = (myPull, myRepoName) => { | ||
| fetch(myPull.commits_url, options) | ||
| .then(res => res.json()) | ||
| .then((commit) => { | ||
| document.getElementById(`commit-${myRepoName}`).innerHTML += ` | ||
| <p> Number of commits: ${commit.length}</p> | ||
| ` | ||
| }) |
There was a problem hiding this comment.
For me, in my project, I think this part was difficult. You've made it excellent!
| } | ||
|
|
||
| .project-container { | ||
| display: grid; |
There was a problem hiding this comment.
Great job to use both flex and grid in this CSS file.
| padding-left: 5vw; | ||
| padding-right: 5vw; | ||
| padding-top: 1vh; | ||
| padding-bottom: 5vh; |
There was a problem hiding this comment.
To make it a little shorter, you've could have wrapped it up in one line.
I also attend to do a lot of unnecessary lines of code:-)
| padding-left: 5vw; | |
| padding-right: 5vw; | |
| padding-top: 1vh; | |
| padding-bottom: 5vh; | |
| padding: 1vh 5vw 5vh 5vw; |
| grid-template-columns: 90vw; | ||
| grid-template-rows: 1fr auto; |
There was a problem hiding this comment.
This seems like something smart, have to read up on grid a little bit.
| plugins: { | ||
| legend: { | ||
| display: true, | ||
| position: 'bottom', |
There was a problem hiding this comment.
Impressed that you found how to positioning the labels at the bottom of the chart.
2 participants
No description provided.