Skip to content

Generate dashboard#1834

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

Generate dashboard#1834
balakhmei wants to merge 2 commits into
mate-academy:masterfrom
balakhmei:develop

Conversation

@balakhmei

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 are still critical functional issues that break the task requirements and would cause tests to fail. First, in src/scripts/main.js you redefine a people array instead of using the imported people from people.json, which violates the requirement to use the provided data source [CHECKLIST ITEM #8]. Second, each table row currently creates too many cells and uses an invalid gender field: you loop over fields = ['name', 'gender', 'born', 'died'] (where person.gender does not exist) and also append genderTd inside that loop, so you end up with extra cells and wrong column order instead of exactly 6 cells: name, gender, born, died, age, century in that order [CHECKLIST ITEMS #2, #3, #9]. You need to build one <tr> per person that appends exactly six <td> elements in the correct order, deriving gender from person.sex once per row, and keep age and century as you already compute them. Also ensure no other files (like workflow configs) are modified; only main.js should change [CHECKLIST ITEM #7]. Once you fix the data source and the exact 6‑cell structure per row, your solution should align well with the requirements, and the good parts you already have (age/century calculations and table selection) will pass the tests.


✨ 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 Outdated
// write your code here
for (const person of people) {
const row = document.createElement('tr');
const fields = ['name', 'gender', 'born', 'died'];

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Using 'gender' in the fields array is incorrect because person has a sex property, not gender. Also, this array should represent the actual columns you want to output; right now it causes attempts to read person['gender'] which is undefined.

Comment thread src/scripts/main.js Outdated
Comment on lines +365 to +368
const genderTd = document.createElement('td');

genderTd.textContent = person.sex === 'm' ? 'Male' : 'Female';
row.appendChild(genderTd);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Creating and appending genderTd inside this loop means a new gender cell is added for every field in fields, leading to too many cells per row and wrong column order. The gender cell should be created once per person, in the correct position relative to name, born, died, age, and century (checklist items #3 and #9).

Comment thread src/scripts/main.js Outdated
Comment on lines +367 to +372
genderTd.textContent = person.sex === 'm' ? 'Male' : 'Female';
row.appendChild(genderTd);

td.textContent = person[key];

row.appendChild(td);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Because you append genderTd on every iteration, and then also append td for each key, you end up with extra cells (more than the required 6 per row). You need exactly 6 cells: name, gender, born, died, age, century, in that order.

@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 fully matches the functional requirements. In main.js you correctly import people from people.json, iterate over the array, and for each person create exactly one <tr> with six <td> cells in the required order: name, gender (derived from person.sex), born, died, age, and century. The age is computed as person.died - person.born and the century with Math.ceil(person.died / 100), which aligns with the task specification. Each row is appended to the table with the .dashboard class, and you have not altered the HTML structure or styling in ways that would break the layout or functionality.

One minor process note: the task expected only main.js to be changed, while you also touched other files earlier in the PR, but the current state no longer has functional or structural deviations, so this is not a blocker. Overall, your code is clear, straightforward, and should pass the automated tests for this task. Nicely done bringing everything in line with the requirements.


✨ 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