Solution#2007
Conversation
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am requesting changes because one core requirement about the employee object schema is not yet met, which can cause tests or consumers of getEmployees to fail. In src/scripts/main.js, inside getEmployees, you currently set name using item.textContent.trim(), which pulls in all text from the <li> (name, position, age, etc.), but the requirement is that name, position, salary, and age are distinct fields, so name must contain only the employee’s name [CHECKLIST ITEM #3]. You should instead read the name from the correct child element or dataset (for example, a span like .employee-name or a data-name attribute) so that name is clean and does not include position or age. Additionally, you are taking salary directly from item.dataset.salary as a string, while you already have a parseSalary helper used for sorting; if the tests expect salary to be numeric in the returned employee objects, you should apply parseSalary when building those objects to keep the schema consistent [CHECKLIST ITEM #3]. Once you adjust getEmployees so that name is correctly isolated and salary uses the same numeric parsing logic, the implementation should fully align with the task requirements. Your sorting logic, use of parseSalary for comparisons, and overall structure of sortList and getEmployees look solid, so with these schema fixes you’ll be in great shape.
✨ 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
| const itemsArray = [...list.children]; | ||
| const employeesArray = itemsArray.map((item) => { | ||
| return { | ||
| name: item.textContent.trim(), |
There was a problem hiding this comment.
item.textContent.trim() will include all text inside the list item (name, position, age, etc.), which violates the requirement that name, position, salary, and age be distinct fields. You should extract only the actual name (e.g., from a dedicated child element or dataset) so name doesn’t contain extra info.
| return { | ||
| name: item.textContent.trim(), | ||
| position: item.dataset.position, | ||
| salary: item.dataset.salary, |
There was a problem hiding this comment.
salary is taken directly from data-salary as a string, while you already have a helper to parse it to a number. If the employee object is expected to hold a numeric salary, consider applying parseSalary here as well to keep the schema consistent and easier to work with.
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am approving your solution because it satisfies all the core requirements and there are no critical blockers. Your sortList(listElement) function correctly reads salary values from data-salary, converts them with a helper to numbers, and sorts the list items in descending order before appending them back to the original list. The getEmployees(list) function returns an array of objects with the required { name, position, salary, age } shape, and you now use item.firstChild.textContent.trim() so the name field is clean instead of being the entire <li> text. You also reuse the same parseSalary helper in both sorting and object creation, ensuring salary is consistently numeric. Changes are properly limited to src/scripts/main.js, and both functions are called at the bottom of the file as required.
The only minor note is that relying on firstChild for the name can be fragile if the HTML structure changes; in a real project, a more explicit selector like a dedicated .employee-name element would be safer. Overall, this is a solid, requirement-compliant implementation—well done.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
DEMO LINK