Skip to content

Solution#1830

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

Solution#1830
Denixify wants to merge 3 commits into
mate-academy:masterfrom
Denixify:develop

Conversation

@Denixify

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 there is one critical requirement not met: the data source. Your table-generation logic (iterating over people, computing age as died - born, computing century with Math.ceil(person.died / 100), and creating 6 <td> cells per row) is all correct and matches the task’s functional requirements. However, in src/scripts/main.js you define a hardcoded const people = [...] array instead of using the people variable that is supposed to be imported from people.json as per the task description [CHECKLIST ITEM #5, #9]. This breaks the requirement that the data must come from the JSON import, and can also conflict with the already imported people variable.

To fix this:

  • Remove the manual const people = [...] definition from main.js (the array with all the people objects).
  • Rely directly on the already imported people variable when generating the table rows (the rest of your logic can stay the same).
    Optionally, after this change, run the required test commands (npm test / npm run test:only variants) to ensure everything passes. Once you switch to using the imported JSON data, your solution should fully meet the task requirements—your existing logic for building the table itself is solid.

✨ 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

Comment thread src/scripts/main.js
Comment on lines +357 to +375
const tableOfPeople = document.querySelector('.dashboard');

// write your code here
people.forEach((person) => {
const age = person.died - person.born;
const century = Math.ceil(person.died / 100);
const gender = person.sex === 'm' ? 'Male' : 'Female';

const row = document.createElement('tr');

row.innerHTML = `
<td>${person.name}</td>
<td>${gender}</td>
<td>${person.born}</td>
<td>${person.died}</td>
<td>${age}</td>
<td>${century}</td>`;

tableOfPeople.append(row);
});

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The rest of the logic that selects the .dashboard table, iterates over people, computes age and century, and appends a row with 6 cells is correct and aligns with checklist items #1#4 and #10#11 once people is coming from the JSON import.

@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 of the task is still not met: the data must come from the imported people.json file, not from a hardcoded array in main.js. Your table-building logic is otherwise correct: you select .dashboard tbody, create one <tr> per person with six <td> cells (name, gender, born, died, age, century), and compute age as died - born and century as Math.ceil(person.died / 100), which matches the specification. The blocking issue is that you still define a large const people = [...] inside src/scripts/main.js, which both violates the requirement and shadows the people variable that should be imported from people.json.

To fix this, remove the entire manual const people = [...] definition from main.js (check around the top of the file where the array is declared) and ensure your people.forEach(...) loop uses the people variable provided by the import statement. After that change, your logic should work as intended with the JSON data source and comply with the task’s constraints. Once updated, run the provided tests (if available) to confirm that everything passes. You’re very close—your DOM manipulation is solid, you just need to hook it up to the correct data source.


✨ 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

@Denixify

Copy link
Copy Markdown
Author
In `main.js`, you already have imported file `people.json`. Variable `people` contains an array of people

##### Steps to do this challenge:
1) For each person from `people` array create table row with 6 table cells (name, gender, born, died, age, century)

Do not change styles or HTML layout in this task. Change only `main.js` file.

The task does not require importing a JSON file

@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.

Sorry, the pull request review was unsuccessful for the following reasons:

Warning

No changes were made in the pull request since the last review. Please make sure that you have added files to the pull request and committed the changes.
[CODE: NCM]

If the issue is within your control, please address it.
The next review attempt will automatically launch in about 1 hour.
If you believe this is an error or need assistance, please contact the support team.

@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.

Sorry, the pull request review was unsuccessful for the following reasons:

Warning

No changes were made in the pull request since the last review. Please make sure that you have added files to the pull request and committed the changes.
[CODE: NCM]

If the issue is within your control, please address it.
The next review attempt will automatically launch in about 1 hour.
If you believe this is an error or need assistance, please contact the support team.

@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 to help you progress with your learning, even though there is one process-related issue to keep in mind. Functionally, your main.js looks solid: you correctly import people from people.json, iterate over that data, and for each person create a <tr> with 6 <td> cells (name, gender derived from sex, born, died, age, century), and append those rows to the table with class dashboard as required [CHECKLIST ITEM #2, #3, #12, #13]. Your age (died - born) and century (Math.ceil(died / 100)) calculations are also correct and match the task specification [CHECKLIST ITEM #5, #6].

The main thing to be aware of for future tasks is that you should avoid modifying index.html when the instructions say to only change main.js [CHECKLIST ITEM #9]; if this happens again, try to revert index.html back to the starter version while keeping the required table headers. As a small improvement, you could make your gender mapping more robust in case future data includes values beyond just "m" and "f". Overall, your core logic is clear, readable, and aligned with the assignment goals—nice work, and keep building on this foundation.


✨ 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