Skip to content

solution#1835

Open
Tysiachnyk wants to merge 4 commits into
mate-academy:masterfrom
Tysiachnyk:develop
Open

solution#1835
Tysiachnyk wants to merge 4 commits into
mate-academy:masterfrom
Tysiachnyk:develop

Conversation

@Tysiachnyk

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 a couple of critical requirement mismatches that are likely to break the tests. First, in src/scripts/main.js you redefine the people array instead of using the people data imported from people.json, whereas the task explicitly requires relying on the provided import; this can cause the tests to run against a different dataset than expected. Second, the second column in the table is supposed to display the original person.sex ('m' / 'f') as the gender column, but your current mapping transforms it to 'Male'/'Female', which does not match the specified data format and will cause column value mismatches in tests. To fix this, remove the inline people definition and rely on the already-imported people, and change your cell creation so that the second cell uses person.sex directly (or a separate gender field if needed, without overwriting sex). Also, ensure that only main.js is modified for this kata (e.g., revert README changes if the instructions say to change only this file). Aside from these points, your logic for computing age, century, and building rows looks solid, so once these issues are corrected your solution should align well with 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

Comment thread src/scripts/main.js Outdated
Comment on lines +361 to +368
const formattedPeople = people.map((person) => {
const age = person.died - person.born;
const sex = person.sex === 'm' ? 'Male' : 'Female';
const century = Math.ceil(person.died / 100);

return {
...person,
sex,

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 mapping adds a sex field transformed to 'Male'/'Female'. The requirements talk about a gender column but do not require changing values; tests typically expect the original sex ('m'/'f') to be displayed. Consider either using the original person.sex directly in the cell or adding a separate gender value without overwriting sex.

@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 because the core functionality is implemented correctly and there are no critical blockers. Your main.js correctly imports the people array, computes age as died - born, computes century using Math.ceil(person.died / 100), and creates a <tr> with six <td> cells in the required order (name, sex, born, died, age, century) for each person. The rows are properly appended to the .dashboard table, so the dashboard should render as required by the task. The build error from Parcel is due to the script tag in index.html not having type="module", which is outside the file you were instructed to modify and therefore not a fault in your main.js logic. The only notable process issue is that you also changed README.md when the task said to modify only main.js; in a real project, you should avoid touching files the instructions mark as off-limits to prevent CI or review conflicts. As a minor improvement, you could slightly simplify the DOM code by appending all <td> elements in a single call like tr.append(tdName, tdSex, ...), but this is not required for correctness. Overall, your implementation meets the functional requirements well—nice work following the previous round of feedback and aligning your code with the specification.


✨ 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 because it correctly builds the dashboard table from the people array, computes age as died - born, and calculates century using Math.ceil(died / 100), with the table columns in the required order. This is an educational override approval, since you also modified files that were supposed to remain unchanged (like index.html and README.md), but the goal here is to let you progress while you learn.

You did a good job using the imported people data and generating one <tr> per person with six <td> cells (name, gender, born, died, age, century), and your small enhancement of mapping gender values like m/f to readable labels is a nice touch. The main issue to improve next time is respecting the constraint to only change main.js: for example, avoid switching the script tag to type="module" in index.html and avoid editing README.md unless the task explicitly tells you to do so. As a minor improvement, you could simplify your DOM code slightly by appending all <td> elements at once (e.g., tr.append(tdName, tdGender, tdBorn, tdDied, tdAge, tdCentury)), but your current approach still works correctly.

Overall, your logic and structure are solid, and this is a strong implementation; keep focusing on both solving the problem and closely following the file/constraint rules in future tasks.


✨ 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