add task solution#2015
Conversation
s-slavets
commented
Jun 28, 2026
- DEMO LINK
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am requesting changes because there is a critical functional issue that breaks the tests and core requirements. Both sortList and getEmployees currently rely on employeeList.employees, but the task specifies that they should work directly with the DOM ul element passed as list, using its children (e.g. list.children or list.querySelectorAll('li')). This leads to a runtime error (TypeError: employeeList.employees is not iterable) and causes the Cypress tests to fail before any assertions can run, so the main functionality is not actually working. To fix this, update sortList(list) so it builds the array of items from list’s <li> elements, sorts them by numeric salary (as you already do), and then appends the sorted <li> nodes back to the same list. Likewise, update getEmployees(list) to iterate over the same DOM children instead of employeeList.employees, mapping each <li> to an employee object using dataset and your getSalary helper. A minor improvement you can consider afterward is extracting the employee name from the specific element that contains it, rather than using the entire li.textContent, which may include other data, but this is not blocking. Your salary parsing, sorting logic, and use of dataset are all solid—once you adjust the functions to use the list’s children directly, your solution should fully meet the 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
| } | ||
|
|
||
| function sortList(employeeList) { | ||
| const sortedList = [...employeeList.employees].sort((a, b) => { |
There was a problem hiding this comment.
employeeList.employees assumes an employees property exists on the list, but the task requires the function to accept the list element itself; use its children (e.g. employeeList.children or querySelectorAll('li')) instead.
| } | ||
|
|
||
| function getEmployees(employeeList) { | ||
| return [...employeeList.employees].map((employee) => { |
There was a problem hiding this comment.
Same issue here: mapping over employeeList.employees will fail unless that property is defined elsewhere; adapt this to work with the DOM list (ul) passed in as the list argument.
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Your solution is approved. The sortList(list) function correctly collects the <li> elements from the ul, sorts them by numeric salary in descending order using the dataset.salary value and your getSalary helper, and re-appends the sorted nodes back to the same list, satisfying the core DOM and sorting requirements [CHECKLIST ITEM #1, #3, #16–#19]. The getSalary helper properly converts the salary from a string data attribute to a number, which is essential for correct sorting and data handling [CHECKLIST ITEM #9, #10, #18]. The getEmployees(list) function iterates over the same DOM children and returns an array of employee objects with the required shape { name, position, salary, age }, using dataset and getSalary as specified [CHECKLIST ITEM #2, #4, #5, #20, #25]. You also respected the constraints about not changing styles or HTML and limiting changes to src/scripts/main.js, and both required functions are called in your script [CHECKLIST ITEM #7, #8, #21, #22, #23, #24].
One minor improvement to consider (not a blocker) is refining how you derive name, since using employee.textContent.trim() may include extra text; targeting a dedicated child element for the name would make the data cleaner [CHECKLIST ITEM #26]. Outside the code, remember to update your demo link with your GitHub username and follow the testing commands from the guidelines, as those process steps remain important even though they aren’t visible in the code. Overall, you met the functional and technical requirements well—nice work on aligning your implementation with the task and previous feedback.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨