Repository navigation
add task solution - #1911
add task solution#1911MarcelloTar wants to merge 3 commits into
Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Your solution requires changes due to critical issues. The Cypress integration tests are failing because the data from people.json is not being rendered in the table. The test expects to find content like '1832' within <td> elements, but your code is using <th> (table header) elements instead of <td> (table data) elements. This is both a semantic HTML issue and a functional blocker - <th> elements are reserved for header cells in the header row, while <td> elements should be used for regular table cells containing data. Replace all <th> tags with <td> in your template literal to fix this and pass the tests.
✨ 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
| tr.innerHTML = ` | ||
| <th>${man.name}</th> | ||
| <th>${sex}</th> | ||
| <th>${man.born}</th> | ||
| <th>${man.died}</th> | ||
| <th>${age}</th> | ||
| <th>${century}</th> |
There was a problem hiding this comment.
The task requires creating 'table cells' but the code uses <th> elements. Table cells should be <td>, not <th>. <th> is semantically reserved for header cells in the header row.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Your solution has a critical issue that is causing the Cypress test to fail. The test expects to find 'Male' text within <td> elements, but it's not present in the table. This indicates that while your code may have the correct number of table cells (6 cells including gender), the actual gender values from the people.json file are not being rendered in the table cells.
Specifically, the failing test should create a table based on people.json file times out because it cannot find the 'Male' content in any <td> element. You need to ensure that the gender column (the second cell in each row) contains the actual gender value from the data (e.g., 'Male' or 'Female'), not an empty cell or incorrect value.
Please verify that your template literal correctly includes the sex or gender property from each person object in the people array when generating the table rows.
✨ 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
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Your solution is approved. The code correctly implements all requirements from the task - it generates table rows from the people array, creates all 6 table cells (name, gender, born, died, age, century), finds the dashboard table, and properly calculates age and century values. The gender conversion from 'm'/'f' to 'Male'/'Female' is working correctly now. Great job fixing the gender display issue! Your implementation is clean and meets every checklist item.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
DEMO LINK