Skip to content

added solution#1842

Open
Skoulskiy wants to merge 2 commits into
mate-academy:masterfrom
Skoulskiy:develop
Open

added solution#1842
Skoulskiy wants to merge 2 commits into
mate-academy:masterfrom
Skoulskiy:develop

Conversation

@Skoulskiy

Copy link
Copy Markdown

No description provided.

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Summary

I am requesting changes because one core requirement is not yet met: the data for the table is hardcoded in src/scripts/main.js instead of being imported from the provided people.json file, which violates the task’s data-source constraint and can break tests that rely on the JSON import. Functionally, your table-building logic is solid: you correctly create a <tr> for each person, add 6 <td> cells in the proper order (name, gender, born, died, age, century), and compute age as died - born and century as Math.ceil(died / 100), which matches the requirements.

Critical issue to fix:

  • Replace the inline people array in main.js with an import from people.json (e.g., import people from '../data/people.json'; or the path used in the starter code). This ensures you follow the task checklist and that tests referencing the JSON file will pass.

Minor improvements (not blocking, but worth considering):

  • The gender column currently maps sex to 'Male'/'Female'; if the tests expect the raw 'm'/'f' values, you can set gender.textContent = pers.sex instead.
  • Variable names like wasBorned and whenDied could be renamed to clearer options such as bornCell and diedCell, and you can remove console.log(people) once you no longer need it for debugging.

Overall, your DOM manipulation and calculations are correct, so once you switch to using people.json for the data source, 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

  1. Rate AI review example

  2. Re-request AI review example

  3. Add AI review comments example

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Summary

Your solution is approved because it now satisfies all core functional and technical requirements for the task. You correctly import people from people.json in main.js, iterate over the array, and for each person create a <tr> with 6 <td> cells in the required order: name, gender, born, died, age, and century, fully meeting items like [CHECKLIST ITEM #1], [#2], and [#5]. The age (pers.died - pers.born) and century (Math.ceil(pers.died / 100)) calculations are exactly as specified, so those computed values will match the expected behavior [CHECKLIST ITEM #3], [#4], [#14], [#15]. You also correctly query the .dashboard table and append each row, and you confined your logic changes to main.js without altering HTML or styles, aligning with the constraints about file modifications.

One small note: you currently map sex to 'Male'/'Female' instead of using the raw 'm'/'f' values; if you ever see a test failing on the gender column, you can switch to gender.textContent = pers.sex to match the data exactly. This is not a blocking issue, and functionally your table will render cleanly and match the intended layout. Overall, you’ve implemented the task correctly and can move forward with this solution—nice work bringing everything in line with the checklist.


✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨

Footnotes

  1. Rate AI review example

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants