marianneardin-githubtracker #98
Conversation
tiiliu
left a comment
tiiliu
left a comment
There was a problem hiding this comment.
Hey Marianne :) Your code was clean and easy to follow. The Javascript functions were very nicely formed. You've met all the blue level requirements, great job! :) This project was definitely challenging, but you made it 💪 💯 🌟
code/index.html
Outdated
| <main id="projects"></main> | ||
| <header> | ||
| <h1>GitHub Tracker</h1> | ||
| <h3 id="username"></h3> |
There was a problem hiding this comment.
I'm not sure what this title or id is referring to? I didn't notice that it would print anything to the page and I didn't see that it was used in the script file either. Maybe it can be removed if unnecessary? :)
code/index.html
Outdated
| <h1>GitHub Tracker</h1> | ||
| <h3 id="username"></h3> | ||
| </header> | ||
| <section class="profile" id="profile"> |
There was a problem hiding this comment.
I noticed that this section has the same class and id names as you have in your script.js file on row 26, where you are printing information about your profile, inside the userContainer element. Perhaps it is referring to those? If that is the case, it is enough that you have those elements in the script file. Using the DOM selector userContainer and then innerHTML you are printing the data inside the userContainer section (row 26 here in html file) and the content of the innerHTML doesn't need to be added here in html. :)
code/index.html
Outdated
| <section class="profile" id="profile"> | ||
| </section> | ||
|
|
||
| <main id="projects"></main> |
There was a problem hiding this comment.
I'm also not sure which element this id is referring to? I wasn't able to find it in the script file. Maybe also removable if not needed? The main tag you could for example use to expand around the main content of your page, semantically it is normally used for specifying the dominant content of a document.
code/index.html
Outdated
| <div class="flex-box"></div> | ||
| <div class="flex-item"></div> |
There was a problem hiding this comment.
I think these are also perhaps forgotten not used elements? I wasn't able to find them in the css file and they seem to be empty :)
code/style.css
Outdated
|
|
||
| } | ||
|
|
||
| .flex { |
There was a problem hiding this comment.
I wasn't able to find an element with this class name, is it used somewhere?
code/style.css
Outdated
| margin-left: auto; | ||
| margin-right: auto; |
There was a problem hiding this comment.
I think these two rows you could combine using: margin: x auto. X (insert right number here) referring to top and bottom margin and auto referring to right and left margin :)
| #chart-wrapper { | ||
| width: 90%; | ||
| margin: 20px auto 40px; | ||
| } |
There was a problem hiding this comment.
You're using the id chartwrapper here, but I couldn't find this id name from the code? You have a class name chart-container created for the element containing the chart (row 34 in html). It is good standard to use class names for styling, so maybe that class name could be used here instead? :)
| margin: 20px auto 40px; | ||
| } | ||
|
|
||
| #chart { |
There was a problem hiding this comment.
Maybe here also would be better to use the class name instead of the id for the chart?
| <div id="profile" class="profile"> | ||
| <div class="profile-image"> | ||
| <a href="${API_URL}"> | ||
| <img src="${data.avatar_url}" alt="Avatar of ${data.login}"> | ||
| </a> | ||
| </div> | ||
| <a href="https://github.com/MarianneArdin"><h1 class="username">${data.login}<h1></a> | ||
| <h2>My projects</h2> | ||
| </div> | ||
| <div> | ||
| <p>Public repositories: ${data.public_repos}</p> | ||
| </div> | ||
| ` |
There was a problem hiding this comment.
Here I noticed that the last div containing the p tag public repositories is outside of the main div (starting from row 26). That means that the styling done with the class name profile isn't included into that div. Perhaps that was meant to be, just wanted to brought this up just in case :)
2 participants
No description provided.