Develop - #2069
Conversation
Marfo0
commented
Sep 8, 2026
Marfo0
commented
- DEMO LINK
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Your implementation has critical issues that prevent it from meeting the task requirements. Both sortList and getEmployees functions must accept a list parameter as explicitly specified in the requirements, but both currently have zero parameters and instead query the DOM internally. Additionally, the task requires you to 'Call both functions' and to 'write helper function for it' for salary conversion - neither of these has been implemented. The ESLint no-unused-vars errors occur because the functions are never called, which is a direct consequence of not following the required function signatures. To fix this: 1) Add list parameter to both functions, 2) Use the passed list parameter instead of document.querySelector internally, 3) Call both functions after defining them, and 4) Extract the salary conversion logic into a helper function as required. The core sorting and employee extraction logic appears sound, so once these structural issues are addressed, the implementation should work correctly.
✨ 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,3 +1,33 @@ | |||
| 'use strict'; | |||
| function sortList() { | |||
There was a problem hiding this comment.
Function sortList should take a list parameter as specified in requirements, but currently has no parameters.
| @@ -1,3 +1,33 @@ | |||
| 'use strict'; | |||
| function sortList() { | |||
| const list = document.querySelector('ul'); | |||
There was a problem hiding this comment.
The list parameter should be used here instead of querying the DOM again. Currently queries document.querySelector('ul') internally instead of using the passed parameter.
| }); | ||
| } | ||
|
|
||
| function getEmployees() { |
There was a problem hiding this comment.
Function getEmployees should take a list parameter as specified in requirements, but currently has no parameters.
| } | ||
|
|
||
| function getEmployees() { | ||
| const workers = [...document.querySelectorAll('li[data-salary]')]; |
There was a problem hiding this comment.
The list parameter should be used here instead of querying the DOM. The function should work with the passed list parameter to extract employees.
| }); | ||
| } | ||
|
|
||
| // write code here |
There was a problem hiding this comment.
Per requirements: 'Call both functions.' - The functions need to be called. The comment 'write code here' suggests this is where the calls should be added.
| const workers = [...document.querySelectorAll('li[data-salary]')]; | ||
|
|
||
| workers.sort((a, b) => { | ||
| const salaryA = Number(a.dataset.salary.slice(1).split(',').join('')); |
There was a problem hiding this comment.
Consider creating a helper function for salary conversion (as mentioned in requirements: 'write helper function for it') since the salary parsing logic is duplicated in both functions.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Your implementation requires changes to meet the task requirements. The sortList and getEmployees functions must accept a list parameter as an argument rather than querying document.querySelector('ul') internally. Additionally, the task requires a named helper function for salary conversion (e.g., getSalary(element)), but this is currently missing and the salary parsing logic is duplicated across your code. Please refactor sortList and getEmployees to accept the list element as a parameter and extract the salary parsing into a reusable helper function.
✨ 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,3 +1,36 @@ | |||
| 'use strict'; | |||
| function sortList() { | |||
There was a problem hiding this comment.
The sortList function must accept a list parameter as specified in the requirements. Currently it has zero parameters but queries the DOM internally instead of using a passed parameter.
| }); | ||
| } | ||
|
|
||
| function getEmployees() { |
There was a problem hiding this comment.
The getEmployees function must accept a list parameter as specified in the requirements. Currently it has zero parameters but queries the DOM internally instead of using a passed parameter.
| const salaryA = Number(a.dataset.salary.slice(1).split(',').join('')); | ||
| const salaryB = Number(b.dataset.salary.slice(1).split(',').join('')); |
There was a problem hiding this comment.
The task requires writing a helper function for salary conversion. Currently the salary parsing logic (Number(a.dataset.salary.slice(1).split(',').join(''))) is duplicated. Extract this into a named helper function like getSalary(element).
| salary: Number( | ||
| worker.dataset.salary.slice(1).split(',').join('') | ||
| ), |
There was a problem hiding this comment.
The salary conversion logic is duplicated here. This should use the same helper function mentioned above.
| }); | ||
| } | ||
|
|
||
| sortList(); |
There was a problem hiding this comment.
When calling sortList, pass the list element as an argument (e.g., sortList(list)). The function should use the parameter instead of querying document.querySelector('ul') internally.
| } | ||
|
|
||
| sortList(); | ||
| getEmployees(); |
There was a problem hiding this comment.
When calling getEmployees, pass the list element as an argument. The function should use the passed parameter to query li elements within that list.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Your solution is approved. All requirements are met: both sortList and getEmployees correctly accept the list parameter as required, the named getSalary helper function properly converts salary strings to numbers, employees are sorted in descending order by salary, and the returned array contains properly structured objects with name, position, salary, and age properties. You also correctly find the list using document.querySelector('ul') and only modify main.js without touching HTML or CSS. Great work implementing these JavaScript DOM manipulation and array methods correctly!
[CHECKLIST ITEMS 1-10 ALL PASS]
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
2 participants
