add solution#2009
Conversation
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am requesting changes because there are still a couple of critical requirement mismatches in getEmployees that will cause the tests to fail. First, on src/scripts/main.js where you build the employee object (inside getEmployees), you currently return salary: item.dataset.salary; this keeps salary as a string, but the task requires a numeric salary converted via your helper, e.g. salary: parseSalary(item.dataset.salary) so that the returned objects match the expected schema [CHECKLIST ITEM #2, #5, #7]. Second, your name field is currently taken from item.textContent.trim(), which will include all text inside the <li> (such as position and possibly salary), not just the employee name; the requirements expect name to come from the dedicated name source (typically item.dataset.name or a specific child element like .employee-name) so the object’s name is accurate [CHECKLIST ITEM #2, #5]. Please update getEmployees to read the name from the appropriate dataset attribute or DOM child, and to use parseSalary when setting salary in the returned object array. Once these two issues are fixed, the rest of your implementation (sorting logic, helper usage, DOM selection) already aligns well with the task, and your solution should pass the tests. Keep going—you’re very close, and these adjustments will make your solution fully compliant 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
| return { | ||
| name: item.textContent.trim(), | ||
| position: item.dataset.position, | ||
| salary: item.dataset.salary, |
There was a problem hiding this comment.
|
|
||
| return items.map((item) => { | ||
| return { | ||
| name: item.textContent.trim(), |
There was a problem hiding this comment.
Using item.textContent.trim() for name will include all text inside the list item, not just the employee name. The task expects an employee object with correct name field, so consider reading the name either from a specific dataset field (e.g., item.dataset.name) or the appropriate child element in the DOM, depending on the HTML structure.
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am approving your solution because it meets all of the core functional requirements of the task. Your parseSalary helper correctly converts salary strings from data-salary into numbers and is reused consistently in both sortList and getEmployees, which matches the requirements. The sortList(listElement) function reads salaries from data attributes, sorts the list items in descending order, and re-appends them to the <ul>, so the visual list ordering will be correct. The getEmployees(listElement) function returns an array of employee objects with the required shape (name, position, salary, age), and importantly, salary is numeric, not a string. The strategy you use to extract name from the <li> text nodes is reasonable given the available HTML context and does not conflict with any explicit requirement.
If the final HTML template exposes a dedicated element like .employee-name or a data-name attribute, you could simplify and harden the name extraction by reading from that instead of manual text-node filtering. Overall, your code is clean, focused, and should satisfy the tests and task expectations—well done.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
No description provided.