Conversation
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am requesting changes because a key requirement of the task is not yet implemented in src/scripts/main.js. You correctly defined getSalary, sortList, and getEmployees, and their internal logic (sorting by salary descending and building the employee objects) looks good. However, the task explicitly requires you to find/get the list from the document and then call both sortList and getEmployees, and currently this file never queries the DOM or invokes these functions, so the behavior never actually runs and tests that rely on those side effects will fail.
Critical issues to fix:
- Add DOM selection for the workers list, for example:
const workerList = document.querySelector('YOUR_LIST_SELECTOR');(use the correct selector for the given HTML). - After obtaining
workerList, call bothsortList(workerList);andgetEmployees(workerList);so that the list is sorted and the employee array is produced as required.
As small, optional improvements, you might add a guard to handle the case where the selector doesn’t find an element (e.g., check if workerList is not null) and consider handling invalid salary values more defensively. Once you wire up the DOM selection and function calls, your solution should align well with the task requirements. Keep going—you’re very close; the main missing piece is just triggering the logic you’ve already implemented.
✨ 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
src/scripts/main.js
Outdated
| age: Number(item.dataset.age) || 0, | ||
| })); | ||
| } | ||
| export { sortList, getEmployees }; |
There was a problem hiding this comment.
The task requires you to find/get the list from the document and call both sortList and getEmployees. This file only defines and exports the functions but never obtains the DOM list or calls them. Add code that selects the list element (e.g. const workerList = document.querySelector('SELECTOR')) and then calls sortList(workerList); getEmployees(workerList); before or after the export so the actions actually run.
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am requesting changes because the current implementation causes the Cypress test for sorting (“should be sorted by salary DSC”) to fail, which means core functionality is not behaving as required and CI remains red. You correctly implemented sortList, getEmployees, and a getSalary helper and wired them to the DOM, but the way salaries are parsed (Number(element.dataset.salary)) is brittle when the dataset contains formatted values (e.g., spaces or commas), leading to NaN and an incorrect descending order.
The critical issue to fix is:
- Robust salary parsing in
getSalary: On the line where you doNumber(element.dataset.salary), normalize the string first (e.g., trim it, remove spaces and commas) and then convert to a number, and ensure you always return a finite number (fallback to0if parsing fails). This will makesortList’s comparator reliable and should satisfy the “sorted by salary descending” requirement.
Minor, non-blocking improvements:
- In
getEmployees, oncegetSalaryis robust, you can rely on it directly (salary: getSalary(item)) instead of using|| 0. - Your
document.querySelector('ul')is fine for this exercise, but in a larger app an ID or class selector for the worker list would be safer.
Overall structure and logic are solid; once you harden the salary parsing so that the list consistently sorts in descending salary order and the Cypress test passes, this solution will fully meet the task 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
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am approving your solution as part of an educational override so you can keep progressing while you learn, even though there are a couple of small refinements still possible. Your getSalary helper is well-designed and robust, and your sortList / getEmployees implementations correctly parse the DOM, sort by salary descending, and return the required { name, position, salary, age } objects, fully satisfying the main task requirements. One minor cleanup: in src/scripts/main.js, inside getEmployees, you currently use salary: getSalary(item) || 0; since getSalary already returns a finite number or 0 as a fallback, you can safely simplify this to salary: getSalary(item) to avoid redundancy. Another small robustness improvement: document.querySelector('ul') on the list selection line will grab the first <ul> on the page; in a larger app it’s safer to give the worker list an id or class and use something like document.querySelector('#workers') so you don’t accidentally target the wrong list. Finally, you’re calling getEmployees(list); without using the returned array—if later logic or tests need that data, consider assigning it, e.g. const employees = getEmployees(list);, so the result is available. Overall, your core logic and structure look solid—these suggestions are small polish points that will make your code cleaner and more maintainable as you keep improving your skills.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| return items.map((item) => ({ | ||
| name: (item.dataset.name || '').trim(), | ||
| position: (item.dataset.position || '').trim(), | ||
| salary: getSalary(item) || 0, |
There was a problem hiding this comment.
getEmployees sets salary: getSalary(item) || 0. Because getSalary already returns a finite number or 0 as fallback, you can simplify this to salary: getSalary(item). This is a small cleanup and aligns with the medium suggestion from the previous review (checklist item #17).
| })); | ||
| } | ||
|
|
||
| const list = document.querySelector('ul'); |
There was a problem hiding this comment.
Selecting the list with document.querySelector('ul') works for the task, but it's brittle in larger pages (it picks the first
- ). Consider using a more specific selector (id or class) for the worker list to avoid accidental selections—this is a robustness recommendation, not a functional blocker.
| if (list) { | ||
| sortList(list); | ||
|
|
||
| getEmployees(list); |
There was a problem hiding this comment.
You invoke getEmployees(list) but do not store its return value. If you or tests need the employees array later, assign it (for example const employees = getEmployees(list);). The current call does invoke the function (satisfying checklist item #7) but discarding the result may be unintended.
No description provided.