Tasks complited - #2056
Conversation
GrizlixXx
commented
Aug 23, 2026
GrizlixXx
commented
- DEMO LINK
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am requesting changes because core task requirements are not implemented as specified. In src/scripts/main.js: (1) The task requires a function sortList(list), but the sorting logic is executed at the top level instead of inside a sortList function. Please wrap the existing sort logic into function sortList(list) { ... return sortedList; } and ensure it accepts a list parameter. (2) The task requires getEmployees(list) that returns an array of employee objects; currently employees is created at the top level. Extract this logic into function getEmployees(list) { ... return employees; } and use the passed-in list. (3) The salary string-to-number conversion using .replace(...) is inline; move this into a helper, e.g., function parseSalary(value) { ... }, and call it inside getEmployees. (4) The task requires calling both functions; update the top-level code to call getEmployees(list) first, then pass the result to sortList(list) (or vice versa as required), and use the returned values.
Minor note: Keep side effects minimal at the top level; rely on returned values from the functions for clarity and testability. You’re close—your logic appears correct, but restructuring into the required functions will satisfy the specification and make the code easier to test and maintain.
✨ 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 sortedBySalary = [...employees].sort((a, b) => { | ||
| return ( | ||
| Number(b.salary.replace('$', '').replace(',', '')) - | ||
| Number(a.salary.replace('$', '').replace(',', '')) | ||
| ); |
There was a problem hiding this comment.
The task requires a function named sortList that takes a list parameter. Currently, there's no such function - the sorting logic is at the top level. Create the required function structure.
| const employees = Array.from(listItems).map((li) => { | ||
| return { | ||
| name: li.textContent.trim(), | ||
| ...li.dataset, | ||
| }; |
There was a problem hiding this comment.
The task requires a function named getEmployees that takes a list parameter and returns an array of employee objects. Currently, employees is created at the top level without being in a function. Extract this into the required function.
|
|
||
| const sortedBySalary = [...employees].sort((a, b) => { | ||
| return ( | ||
| Number(b.salary.replace('$', '').replace(',', '')) - |
There was a problem hiding this comment.
The task requires a helper function for converting salary string to number. The .replace('$', '').replace(',', '') logic is inline but should be extracted into a helper function as specified.
| list.innerHTML = sortedBySalary | ||
| .map((employee) => { | ||
| return ` | ||
| <li | ||
| data-position="${employee.position}" | ||
| data-salary="${employee.salary}" | ||
| data-age="${employee.age}" | ||
| > | ||
| ${employee.name} | ||
| </li> | ||
| `; | ||
| }) | ||
| .join(''); |
There was a problem hiding this comment.
The task requires calling both functions. The current code executes at the top level without proper function calls to sortList and getEmployees.
|
Я поспорю з тобою і скажу що то була рекомендаційна інструкція тобто я не мусив робити функцію для сортування і інших дій то просто підсказка як можна було зробити завдання але я вирішив його зробити по свойому код працює програма виконує одинаковий результат тому не бачу причин відмовляти мені я вважаю що мій код кращий ніж запропонований в інструкції |
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Sorry, the pull request review was unsuccessful for the following reasons:
Warning
No changes were made in the pull request since the last review. Please make sure that you have added files to the pull request and committed the changes.
[CODE: NCM]
If the issue is within your control, please address it.
The next review attempt will automatically launch in about 1 hour.
If you believe this is an error or need assistance, please contact the support team.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Your implementation correctly meets all requirements. The employee objects properly derive their name, position, salary, and age properties from li.dataset through the spread operator. While there's a redundant line setting name: li.textContent.trim() before the spread, it has no functional impact since the spread overwrites it with the correct li.dataset.name value. The parseSalary, sortList, and getEmployees functions all work as specified. Great work on completing this task!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| return listItems.map((li) => ({ | ||
| name: li.textContent.trim(), |
There was a problem hiding this comment.
The name property is being set to li.textContent.trim() which includes all visible text content (name + position + salary + age). Since li.dataset already contains name, position, salary, and age as separate properties, the employee object should use li.dataset.name instead of text content.
2 participants
