Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Great job getting the core sorting logic to work correctly! The list is sorted, and the employee data is extracted as required. However, a few changes are needed to fully align with the task's structural requirements.
Specifically, there are two main issues:
- The
sortListfunction should also be responsible for appending the sorted items back to the list in the DOM. Currently, this logic is handled in the global scope and should be moved inside the function. - The task requires a dedicated helper function to convert the salary string to a number. Your conversion logic is correct, but it needs to be extracted into a separate function.
Additionally, there is a redundant call to sortList at the end of the script that can be removed.
You're very close to a complete solution! Please make these adjustments to meet all 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
src/scripts/main.js
Outdated
| const ul = document.querySelector('ul'); | ||
| const list = [...document.querySelectorAll('li')]; | ||
|
|
||
| function sortList(li) { |
There was a problem hiding this comment.
According to the task description, the sortList function should also be responsible for appending the sorted items back to the list in the DOM. Currently, the appending logic is handled in the global scope (line 17). Consider moving that logic inside this function.
src/scripts/main.js
Outdated
|
|
||
| function sortList(li) { | ||
| return li.sort((el1, el2) => { | ||
| const salaryEl1 = Number(el1.dataset.salary.replace(/[$,]/g, '')); |
There was a problem hiding this comment.
The requirements state that you should create a helper function for converting the salary string to a number. While your conversion logic here is correct, it should be extracted into a separate function to meet the task's requirements.
src/scripts/main.js
Outdated
| })); | ||
| } | ||
|
|
||
| sortList(list); |
There was a problem hiding this comment.
This call to sortList is redundant. The list was already sorted by the call on line 15, and the Array.prototype.sort() method sorts the array in place, so the list variable is already in the correct order.
| } | ||
|
|
||
| sortList(list); | ||
| getEmployees(list); |
There was a problem hiding this comment.
This function is called correctly, but its return value (the array of employee objects) is never used. For this task, simply calling it is sufficient, but in a real-world scenario, you would want to store this result in a variable for later use.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Excellent work on this task! I am approving your solution as you've successfully addressed all the feedback from the previous review. The sortList function now correctly appends the sorted elements back to the DOM, and the new helper function to parse the salary is a great addition for clarity and reusability. Your code is clean, functional, and fully meets all the project requirements. Keep up the great work!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
2 participants
No description provided.