Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Great work on calculating the age and century correctly! The data displayed in the table is accurate.
However, one change is needed regarding how you update the table. You are appending the row to the table after adding each individual cell. This is inefficient as it causes the browser to re-render the page multiple times for a single row.
To fix this, you should construct the entire row with all its cells first. Then, append the completed row to the table body only once at the end of the loop for each person. This small change will significantly improve your code's performance.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Important
Fix the issues, commit the changes, and then re-request my review using the Re-request review button2 in the Reviewers tab.
Tip
If you have any comments about this review, please leave a message in the Add a comment section below and mention me with the @mateacademy-ai-mentor tag3.
I will read your message during my next review of your pull request. Please do not reply directly to my comments, as that feature is not supported yet.
Caution
Do not close the pull request until the review is complete. Closing it early can lead to unexpected behavior and disrupt the review process.
Footnotes
src/scripts/main.js
Outdated
| table.append(row); | ||
|
|
||
| cell = document.createElement('td'); | ||
|
|
||
| if (person.sex === 'f') { | ||
| cell.textContent = 'Female'; | ||
| } else { | ||
| cell.textContent = 'Male'; | ||
| } | ||
| row.append(cell); | ||
| table.append(row); | ||
|
|
||
| cell = document.createElement('td'); | ||
| cell.textContent = person.born; | ||
| row.append(cell); | ||
| table.append(row); | ||
|
|
||
| cell = document.createElement('td'); | ||
| cell.textContent = person.died; | ||
| row.append(cell); | ||
| table.append(row); | ||
|
|
||
| cell = document.createElement('td'); | ||
| cell.textContent = agePerson; | ||
| row.append(cell); | ||
| table.append(row); | ||
|
|
||
| cell = document.createElement('td'); | ||
| cell.textContent = century; | ||
| row.append(cell); | ||
| table.append(row); |
There was a problem hiding this comment.
You are appending the row to the table every time you add a new cell. While the final output might look correct, this is inefficient because each .append() call to an element that is part of the document can cause the browser to recalculate layouts and repaint the screen.
A better practice is to build the entire row with all its cells first, and then append the fully constructed row to the table only once at the end of the loop for each person.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Excellent work on this iteration! You've successfully addressed the performance issue from the last review, and your code now correctly implements all the requirements. I am approving your solution. For future improvements, consider making your table selector more specific with document.querySelector('.dashboard tbody') to make it more robust. Also, I noticed a small typo in a variable name that could be corrected for better readability. Keep up the great work!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
src/scripts/main.js
Outdated
| // console.log(people); // you can remove it | ||
|
|
||
| // write your code here | ||
| const table = document.querySelector('tbody'); |
There was a problem hiding this comment.
The task description asks you to find a table with the class dashboard. While document.querySelector('tbody') works for the current page structure, it's not very robust. If another table were added to the page, your code might select the wrong <tbody>. A more precise selector, like document.querySelector('.dashboard tbody'), would ensure you're always targeting the correct table.
src/scripts/main.js
Outdated
| cellPerson.textContent = agePerson; | ||
| row.append(cellPerson); | ||
|
|
||
| const cellCentry = document.createElement('td'); |
There was a problem hiding this comment.
It looks like there's a small typo in this variable name. It should likely be cellCentury.
2 participants
No description provided.