Week 7 github-tracker #107
Conversation
…nd default branch,link to github projects on the page
…d my structure in HTML
nadialefebvre
left a comment
nadialefebvre
left a comment
There was a problem hiding this comment.
Terese, you did a really good job, especially since you wrote in your README that you didn't know at all what to do ;) Your JavaScript is clear and simple. I left many times the same little comment in your CSS, to show you some of the places that you are repeating yourself in the media queries. When a property stays the same on tablet and/or desktop view, no need to repeat it. You should only add what is changed from one view to another. In conclusion, it seems to me that you did everything that was required for this project. You can be proud of yourself and of all your learning process doing so!
| padding: 3rem; | ||
| text-align: center; | ||
| font-size: 1.5em; |
There was a problem hiding this comment.
I'm wondering if there's a reason why you used both rem and em for sizes in your CSS? I would be more inclined to use the same unit everywhere for more uniformity, but maybe you had a reason for doing it like that.
| background: #FFECE9; | ||
| } | ||
| margin: 0; | ||
| background: #e3ded9; |
There was a problem hiding this comment.
A tip about colors: adding color variables in your CSS would make it really easy to use your colors at many places in the file. It would help you to keep consistency (it's currently saving my life in the portfolio project). You add this at the beginning or your CSS (with your colors and variable names of course):
:root {
--white: #ffffff;
--primary: #fa382f;
--secondary: #072bce;
}
And when you use the colors, you just type color: var(--primary); so if you want to change the colors later, it's only in the root part that you need to make a change. It's way easier to keep track of the colors used.
| flex-direction: column; | ||
| } | ||
|
|
||
| .projects div { |
There was a problem hiding this comment.
Maybe adding a min-width would improve a little bit the appearance of your repo divs, because they are now shrinking/stretching depending on the content.
| .profile img { | ||
| border-radius: 50%; | ||
| max-height: 30rem; | ||
| max-height: 30rem; |
| font-size: 1em; | ||
| } | ||
|
|
||
| @media screen and (min-width: 678px) and (max-width: 1024px) { |
There was a problem hiding this comment.
If you remove the and (max-width: 1024px), it wouldn't change anything here so since less is better... ;)
| display: flex; | ||
| flex-direction: row; | ||
| flex-wrap: wrap; | ||
| justify-content: space-around; |
There was a problem hiding this comment.
No need to repeat it here, since it's not a change from the tablet version (already written in this section).
| `; | ||
| commits(repo.commits_url, repo.name); | ||
| }); | ||
| getPullRequest(forkedRepos); |
There was a problem hiding this comment.
Great that you created a function and called it here, because it makes your code clearer.
|
|
||
| const getPullRequest = (forkedRepos) => { | ||
| forkedRepos.forEach((repo) => { | ||
| const PULL_URL = `https://api.github.com/repos/Technigo/${repo.name}/pulls?per_page=100`; |
There was a problem hiding this comment.
Good thing that you added perpage=100 in the URL because some pulls will be quickly missed with the default (30 results/page).
| } | ||
| }); | ||
|
|
||
| if (groupProject === true) { |
There was a problem hiding this comment.
You found a good solution for the projects where you weren't the one creating the pull request.
| let commitUrl = myCommits.replace("{/sha}", ""); | ||
| fetch(commitUrl) | ||
| .then((res) => res.json()) | ||
| .then((commitNumber) => { |
There was a problem hiding this comment.
I'm wondering if the use of commitNumber could lead to some misunderstanding here, since it represents in fact an array of commits, not the number of commits?
2 participants
No description provided.