Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am requesting changes because one core requirement is not met and this counts as a critical blocker for the task. In src/scripts/main.js, you currently define the full people array inline (around the top of the file) instead of using the already-imported people.json / provided people variable, which violates the requirement to rely on the given data source [CHECKLIST ITEM #9]; please remove the inline array and use the imported/provided people data so tests and structure match the spec. Also, when you select the table with document.querySelector('.dashboard'), add a guard like if (!table) return; (or log an error) before you start creating/appending rows, so the script doesn’t throw if the element is missing. When appending rows, check for a <tbody> first, e.g. const tbody = table.querySelector('tbody') || table; and append tr elements to tbody instead of always calling table.append(tr). Aside from these issues, your logic for creating rows and computing age and century looks correct and aligns well with the task, so once you fix the data source and add the defensive checks, the solution should be in good shape.
✨ 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
| const table = document.querySelector('.dashboard'); | ||
|
|
||
| // write your code here | ||
| people.forEach((person) => { |
There was a problem hiding this comment.
The forEach iterates over the people variable declared in this file. The task requires using the already-imported people.json / provided people variable instead of defining the array inline. Remove the inline declaration and import or use the provided people data so the tests use the expected source (this is a major checklist violation - checklist item #9).
|
|
||
| // eslint-disable-next-line no-console | ||
| console.log(people); // you can remove it | ||
| const table = document.querySelector('.dashboard'); |
There was a problem hiding this comment.
You're selecting the .dashboard table but don't verify it exists before appending rows. Add a defensive check like if (!table) return; or an explicit error to avoid runtime exceptions when the element isn't found (improves robustness; relates to checklist item #10).
src/scripts/main.js
Outdated
| tdCentury.textContent = century; | ||
|
|
||
| tr.append(tdName, tdGender, tdBorn, tdDied, tdAge, tdCentury); | ||
| table.append(tr); |
There was a problem hiding this comment.
You append rows directly to table with table.append(tr). If the table markup includes a <tbody> you might prefer appending to table.querySelector('tbody') when present. Also ensure the table selection check above runs before this append to prevent errors.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am requesting changes because there is a critical requirement violation regarding the data source. In src/scripts/main.js you define a full inline const people = [...] array, but the task explicitly requires using the already provided/imported people data (from people.json or the given variable), not redefining it locally. This means your code is not using the official data source and will likely cause the automated tests and expectations to fail, even though your row creation, age calculation (died - born), and century calculation (Math.ceil(died / 100)) are implemented correctly.
To fix this:
- Remove the inline
peoplearray definition frommain.js(look for the large array near the top of the file). - Use the provided
peoplevariable instead (the one that the starter code or import already exposes tomain.js). - Keep your existing logic that builds
<tr>with six<td>cells, computes age and century, and appends rows to thetbodyof the.dashboardtable, just change the data source to the providedpeople.
After making this change, runnpm run testto ensure everything passes. Your table-building logic is solid, so once the data source is corrected, your solution should be in good shape.
✨ 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
I am approving your solution as part of an educational override so you can keep progressing while you learn, even though there are a couple of process issues to fix next time. Your main.js correctly imports the people data, creates one <tr> per person, and fills in all six required columns (name, gender, born, died, age, century) with the right age and century calculations, which shows good understanding of both the task and basic DOM manipulation. The main requirement you missed is the “change only main.js” constraint: your pull request also includes built dist files and a modified src/index.html, which goes against the rule to avoid committing build artifacts and to leave the HTML layout unchanged—next time, make sure those files are either reverted or added to .gitignore.
A smaller improvement is in your gender handling: right now the code treats any value that isn’t 'm' as female, which could mislabel unexpected values; you can make this more robust by explicitly mapping 'm' to "Male", 'f' to "Female", and using a safe fallback for anything else. It would also be slightly more semantically correct to ensure there is a <tbody> element on the table (and create one if missing) before appending rows, rather than appending rows directly to the <table>. Overall, your core logic and structure are solid, and with a bit more attention to repo hygiene and edge cases, you’re very close to a polished, production-ready solution—well done, and keep building on this foundation.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
2 participants
No description provided.