Skip to content

add task solution#1745

Open
freelinex wants to merge 2 commits intomate-academy:masterfrom
freelinex:develop
Open

add task solution#1745
freelinex wants to merge 2 commits intomate-academy:masterfrom
freelinex:develop

Conversation

@freelinex
Copy link

Copy link

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

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 of the core checklist requirements is not met, even though your table-building logic and calculations look good. The main critical issue is that src/scripts/main.js defines the people array inline instead of importing it from people.json, which violates the requirement to use the JSON file as the data source ([CHECKLIST ITEM #8]); this needs to be fixed by adding an import like import people from './people.json'; (with the correct relative path) and removing the hardcoded array. A second issue tied to the same checklist is that you select '.dashboard tbody' directly; the task explicitly asks to “find the table with class dashboard” ([CHECKLIST ITEM #10]), so you should first get the table via document.querySelector('.dashboard') and then its tbody via table.querySelector('tbody'). When you change the selector, also add a small safety check such as:

const table = document.querySelector('.dashboard');
if (!table) {
  console.error('dashboard table not found');
  return;
}
const tbody = table.querySelector('tbody');

so your code doesn’t throw if the table is missing. Once you’ve imported people.json correctly and updated the table selection with a null check, your implementation should fully match the functional requirements, and the remaining CI error related to @parcel/watcher will be a tooling issue rather than a problem with your code. Overall, your age and century computations and row construction are solid, and these changes will bring the solution fully in line with the checklist.


✨ 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

const dashboard = document.querySelector('.dashboard tbody');

// write your code here
people.forEach((item) => {

Choose a reason for hiding this comment

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

The code uses a locally defined people array instead of importing people.json as required by checklist item #8. Import the JSON (for example import people from './people.json' with the correct path) and use that imported variable so the implementation matches the task requirements.

Copy link
Author

Choose a reason for hiding this comment

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

The task did not require importing the people.json file, as it had already been imported into the file beforehand. Moving it to a separate file contradicts the point: "Do not change styles or HTML layout in this task. Change only the main.js file."


// eslint-disable-next-line no-console
console.log(people); // you can remove it
const dashboard = document.querySelector('.dashboard tbody');

Choose a reason for hiding this comment

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

You are selecting the tbody directly with '.dashboard tbody'. The requirement (#10) asks to find the table with class dashboard. Consider selecting the table (document.querySelector('.dashboard')) and then its tbody (e.g. table.querySelector('tbody')) so the code explicitly finds the table element as specified.

<td>${Math.ceil(item.died / 100)}</td>
`;

dashboard.appendChild(row);

Choose a reason for hiding this comment

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

Appending rows without verifying the dashboard element exists may throw if the selector fails. Consider checking if (!dashboard) { console.error('dashboard table not found'); return; } before calling appendChild to avoid runtime errors when the DOM isn't as expected.

Copy link

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

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 part of an educational override so you can keep progressing while you learn.

You did a solid job generating the table rows: all six cells (name, gender, born, died, age, century) are present and the formulas for age and century are correctly implemented. To fully match the task requirements, you still need to remove the hardcoded people array in src/scripts/main.js and instead import the data from people.json (e.g. import people from './people.json';) so the script uses the provided data source [CHECKLIST ITEM #8]. You also need to change the table selection so you first get the table with document.querySelector('.dashboard'), check that it exists, then get const tbody = table.querySelector('tbody') and append your generated rows to tbody instead of using document.querySelector('.dashboard tbody') directly [CHECKLIST ITEM #10]. These changes will make your solution more robust and aligned with the checklist, but your current work shows good understanding of DOM manipulation and computed values.

Keep going—once you adjust the data import and table selection pattern, your implementation will be fully in line with the assignment’s expectations.


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

Footnotes

  1. Rate AI review example

const dashboard = document.querySelector('.dashboard tbody');

// write your code here
people.forEach((item) => {

Choose a reason for hiding this comment

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

Blocking: You iterate people here, but in this file people is defined inline (the hardcoded array earlier). The checklist requires using the imported people.json as the data source (checklist item #8). Remove the hardcoded array and import the JSON instead, e.g. import people from './people.json'; (use the correct relative path).


// eslint-disable-next-line no-console
console.log(people); // you can remove it
const dashboard = document.querySelector('.dashboard tbody');

Choose a reason for hiding this comment

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

Blocking: This line selects '.dashboard tbody' directly. The task explicitly asks to "find the table with class dashboard" first (checklist item #10). Change this to select the table (document.querySelector('.dashboard')), add a null check, then get its tbody via table.querySelector('tbody') and use that tbody to append rows.

<td>${Math.ceil(item.died / 100)}</td>
`;

dashboard.appendChild(row);

Choose a reason for hiding this comment

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

After you switch to selecting the table + tbody, append rows to the tbody variable (for example tbody.appendChild(row)) and not to the old dashboard variable. Also keep the small safety null-check so the script doesn't throw if the table is missing.

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