homework week1 is done #56
Conversation
profxed
added
homework1
to review
class16
review in progress
and removed
to review
labels
profxed
left a comment
profxed
left a comment
There was a problem hiding this comment.
Hi @genegr88, you have a very nice clean code, I am very happy to see your following for the videos of week1, even If you missed a point of the requirements, you did great job.
- I really Liked the
renderRepositoryfunction as a DRY way of improving the flexibility of your program. - you still have an issue with two main things such as:
- naming conventions
- you have clean code but not really Indented very well you have to follow one specific kind f indentation so for instance you have in some lines 3 spaces while you have in others 4, and in the file scope you have the Brackets surrounded your code
{/* your code... */}so you have to indent inside of this scope as well.
I hope I didn't miss any thing 🙂.
| function main(url) { | ||
| const root = document.getElementById('root'); | ||
| const divForSelect = createAndAppend('div', root, {id: "container-select"}); | ||
| const heading = createAndAppend('h5', divForSelect, {text: "HYF Repositories", class: "heading"}); |
There was a problem hiding this comment.
heading in my vs.code is gray which means you assigned but never used it.
Assigning variable is a good thing for clarifying things out 🙂, but it's a memory lake, so putting a comment near to it would be nice and less memory effort.
| { | ||
| const URL_Rep = 'https://api.github.com/orgs/HackYourFuture/repos?per_page=100'; | ||
| function renderRepository(container, repository) { | ||
| let someDate = new Date(repository.updated_at); |
There was a problem hiding this comment.
someDate is a meaningful name, for you 😸, I found this thing actually that we are give a very meaningful names for us but when somebody else reads our code will face an issue understanding what is referring to.
So someDate of course it's some date but it would be more clear to call it lastUpdate or latestCommit or you may have a better name for it.
so the point is this variable handling a date for an event accord in the past, you can call it as the event called simply 🙂 .
| const tBody = createAndAppend("tbody", innerTable); | ||
| const tr = createAndAppend('tr', tBody); | ||
| createAndAppend('td', tr, {text: "Repository:", class: "nameRow"}); | ||
| const tdForLink = createAndAppend('td', tr,); |
There was a problem hiding this comment.
it's not a valid syntax to have an empty field after the last comma of your function call, nothing will happen but it may have some confusion, so if I came after a year or so, for the first look here I will say something was here and removed.
and the name you can Just call it repositoyLink in case you want to refer that it's a <td> tag you can just call it td_repositoyLink or as always you may have a better one 💯
3 participants
homework the first part is finished