Skip to content

add task solution#1836

Open
mariiaherasymiukdev-bit wants to merge 3 commits into
mate-academy:masterfrom
mariiaherasymiukdev-bit:develop
Open

add task solution#1836
mariiaherasymiukdev-bit wants to merge 3 commits into
mate-academy:masterfrom
mariiaherasymiukdev-bit:develop

Conversation

@mariiaherasymiukdev-bit

Copy link
Copy Markdown

@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 the automated Cypress test that checks table generation is failing, which means a core requirement of rendering the data from people.json into the table is not fully met. Your main.js logic for iterating over people, creating <tr> and <td> elements, and computing age and century is correct and matches all listed checklist items for structure and calculations. However, the failing test (should create a table based on people.json file) indicates that, in the actual running app, the table cells are not showing values like 'Male', so the DOM is not being populated as the test expects.

Critical issues to fix:

  • Ensure that the people data used in main.js is actually loaded from people.json in the running page (for example, via an import or a <script> that defines people before main.js runs), so that people.forEach(...) is iterating real data and not an undefined/empty value.
  • Confirm that you are appending your generated rows to the same table/element that the tests target (the .dashboard table body). If the HTML changed (e.g., different class, using <tbody> instead of the root table without adjusting selectors), the test would not find the cells even if they are created.

Once you ensure people is correctly wired to people.json and that rows are appended into the expected element so that values like 'Male' appear in <td> cells, the failing Cypress test should pass. Apart from this data-loading/wiring issue, the DOM generation logic itself looks solid, so you are very close—fixing this integration detail will complete the task.


✨ 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

I am requesting changes because your solution currently redefines the people array directly in src/scripts/main.js instead of using the already imported data from people.json, which is a core requirement of the task. This is a critical blocker: tests and task instructions expect the external people variable to be used as-is, and overriding it can cause mismatches with the provided data and automated checks. To fix this, remove the hard-coded const people = [...] declaration in main.js (around the top of the file) and rely solely on the people variable that is imported from people.json. After that change, keep your existing logic that iterates with people.forEach, creates <tr> and <td> elements, and fills in name, gender, born, died, age (died - born), and century (Math.ceil(died / 100)) before appending to the .dashboard table. As a minor improvement, you can also remove or comment out console.log(people) once you’ve finished debugging, to keep production code clean. Your DOM-building logic is otherwise solid, so once you hook it up to the imported data correctly, your solution should fully meet the task requirements.


✨ 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

I am approving your solution as an educational override so you can keep progressing while you learn, even though there is still one important issue to address. Your DOM logic is well done: you correctly find the .dashboard table, loop through people, build a <tr> with six <td> cells, and compute both age (died - born) and century (Math.ceil(died / 100)), which covers the main functional requirements [CHECKLIST ITEM #1, #2, #5, #6, #7, #8, #9].

The main thing to improve is how you get the data: in src/scripts/main.js you currently declare a local const people = [...] (around the bottom of the file), which overrides the people array that is supposed to be imported from people.json [CHECKLIST ITEM #3, #15]. This is a problem because the task requires using the provided JSON data as-is, and tests will rely on that import, so your hard‑coded array can easily get out of sync. To fix this, remove the entire const people = [...] block and keep using people.forEach(...) exactly as you do now; the only difference is that people should come from the existing import at the top of main.js, not from a new declaration.

You’ve already handled the table generation cleanly and kept extra console output commented, which is good practice, so once you adjust the data source you’ll have a solid, requirements-compliant solution. Keep going—this is a great step in practicing DOM manipulation and working with imported data.


✨ 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