first update#2018
Conversation
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 several core requirements of the task are not yet implemented, which will block the solution from passing the tests. You correctly locate the list and sort the <li> elements by data-salary in descending order, then append them back, but this logic is currently in the global scope instead of inside the required sortList(list) function. The task explicitly requires a helper function to convert the salary string to a number, yet this conversion is duplicated inline rather than extracted into a reusable helper (e.g. getSalary(li)), so you should move that logic into a dedicated function and use it in your sorting. The getEmployees(list) function is missing entirely; you need to implement it so that it iterates over the li elements of the given list and returns an array of objects with the exact shape { name, position, salary, age }, using the text content and data-* attributes. After querying the list element, both sortList(list) and getEmployees(list) must be called so the required behavior is triggered via the specified API instead of ad-hoc code. As minor improvements, consider using clearer variable names and optional comments to show how each part of the DOM maps to the employee object fields. Once these functions are implemented and properly invoked, your solution should align with the task requirements and be in good 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 elementsArray = Array.from(dataitems); | ||
| const sortingitems = elementsArray.sort(compareSalary); |
There was a problem hiding this comment.
The requirements ask for a sortList(list) function, but you currently perform sorting directly in the global scope and don't define or use sortList that takes list as a parameter.
| const salarya = Number(a.dataset.salary.replace('$', '').replaceAll(',', '')); | ||
| const salaryb = Number(b.dataset.salary.replace('$', '').replaceAll(',', '')); |
There was a problem hiding this comment.
The task description explicitly asks for a helper function to convert salary strings to numbers; here the conversion logic is duplicated inline instead of being moved into a dedicated helper.
| const elementsArray = Array.from(dataitems); | ||
| const sortingitems = elementsArray.sort(compareSalary); | ||
|
|
||
| sortingitems.forEach(moveToEnd); |
There was a problem hiding this comment.
The getEmployees(list) function required by the task is missing; you need to create it so that it returns an array of { name, position, salary, age } objects derived from the list items.
| const elementsArray = Array.from(dataitems); | ||
| const sortingitems = elementsArray.sort(compareSalary); | ||
|
|
||
| sortingitems.forEach(moveToEnd); |
There was a problem hiding this comment.
Both sortList and getEmployees are supposed to be called, but currently only the ad-hoc sorting logic is run; make sure to call these functions with the list element.
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 there are still a couple of critical mismatches with the task specification that will likely break the tests. First, the sorting function is implemented as sortlist(list) instead of sortList(list), but the task and tests expect the exact name sortList(list); this prevents the required API from being found and used, so it’s a functional blocker. Second, the salary conversion logic is duplicated inline in both getEmployees and the comparator instead of being extracted into a dedicated helper function as explicitly required by the description and [CHECKLIST ITEM #8]; tests may check for this helper and currently will not find it. There is also an ESLint no-unused-vars error for likeresult (around line 38 in src/scripts/main.js), which will block the build and should be resolved by either removing the variable or actually using it. To fix the critical issues, rename sortlist to sortList and update all calls, and introduce a small helper like function getSalary(li) { return Number(li.dataset.salary); } that you reuse both when building employee objects and inside the sorting comparator. As a smaller improvement, verify how name is stored in the HTML and, if it’s provided via a data-name attribute, switch from li.textContent.trim() to li.dataset.name so your employee objects match the underlying markup. Overall, you’re very close—once these changes are made, your solution should align well with the requirements and pass the tests.
✨ 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 items = list.querySelectorAll('li'); | ||
|
|
||
| return Array.from(items).map(function(li) { | ||
| const clearsalary = Number(li.dataset.salary.replace('$', '').replaceAll(',', '')); |
There was a problem hiding this comment.
The task requires a helper function to convert salary from the data attribute string to a number; here you duplicate that logic inline instead of reusing a dedicated helper, which can violate checklist item #8 and cause tests expecting a helper function to fail.
| } | ||
|
|
||
| function compareSalary(a, b) { | ||
| const salarya = Number(a.dataset.salary.replace('$', '').replaceAll(',', '')); |
There was a problem hiding this comment.
Similarly, salary conversion logic is duplicated here instead of using a shared helper function, which the description explicitly asks you to create for converting salary strings to numbers.
| return salaryb - salarya; | ||
| } | ||
|
|
||
| function sortlist(list) { |
There was a problem hiding this comment.
The sorting function is named sortlist, but the task and tests expect a function sortList(list); this name mismatch will prevent tests or other code from finding and using the required API.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am approving your solution, and this approval is given as an educational override so you can keep progressing while you learn. Functionally, your sortList and getEmployees implementations look solid: you correctly build employee objects with { name, position, salary, age }, convert the salary from the data attribute to a number with a helper, and sort the list in descending order by salary as required. One issue flagged by ESLint is that the local variable name (around line 11) shadows the browser global window.name, which violates the no-shadow rule; renaming it to something like itemName or employeeName will fix the lint error and improve clarity. This doesn’t break your functionality, but it does block the automated checks, so it’s a good habit to avoid using names that conflict with globals. Overall, you’ve done a strong job meeting the task requirements and cleaning up previous review points, and with that small naming adjustment your code should be both correct and style-compliant. Keep up the good work—this kind of attention to helpers, data attributes, and clean sorting logic is exactly what will help you grow as a developer.
✨ 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
3 participants
No description provided.