first commit #1891
first commit #1891
Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Good work on this task! The code is well-structured and the data parsing is implemented correctly. However, I'm requesting a couple of changes to meet the requirements.
The most important issue is that the list is sorted in ascending order, but the task requires descending order. You'll need to adjust the comparison logic inside your .sort() function to fix this.
Also, you can make your code cleaner by reusing your getSalary helper function inside the getEmployees function. This will avoid duplicating the salary parsing logic.
You're very close to a complete solution. Keep up the great work!
✨ 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
| function sortList(list) { | ||
| const itemsArray = Array.from(list.children); | ||
|
|
||
| itemsArray.sort((a, b) => getSalary(a) - getSalary(b)); |
There was a problem hiding this comment.
The sorting logic here is for ascending order (a - b). The requirement is to sort the list in descending order. You'll need to adjust the comparison to achieve the correct order.
src/scripts/main.js
Outdated
| return itemsArray.map((item) => ({ | ||
| name: item.textContent.trim(), | ||
| position: item.dataset.position, | ||
| salary: Number(item.dataset.salary.replace(/[$,]/g, '')), |
There was a problem hiding this comment.
This line duplicates the logic that's already in your getSalary helper function. To avoid repetition and make your code easier to maintain, you should reuse the helper function here: salary: getSalary(item).
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 iteration! You've correctly implemented the descending sort order and are reusing the helper function effectively. I am requesting one final change to fully meet the task requirements. The sortList and getEmployees functions must accept the list element as an argument, but they currently rely on a variable from the outer scope. Please update their definitions to accept a parameter, like so:
function sortList(list) {
// ...
}
function getEmployees(list) {
// ...
}Then, make sure to pass the list variable when you call these functions. This change will make your functions more reusable and fully align with the task requirements. You're very close!
✨ 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
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am approving this submission as an educational override to help you progress. You've done a great job updating the function signatures to accept a parameter, which is a key step toward making your code more reusable! The final step is to ensure the functions use the parameter they receive. Inside sortList and getEmployees, replace the global list variable with the listElement parameter. Also, remember to pass the list variable when you call the functions, for example: sortList(list). You are very close to a perfect solution, keep up the excellent work!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| } | ||
|
|
||
| function sortList(listElement) { | ||
| const itemsArray = Array.from(list.children); |
There was a problem hiding this comment.
You've correctly added the listElement parameter to the function, but here you're still using the global list variable. To make the function reusable as intended, you should use the listElement parameter for all operations within this function, including on lines 14 and 16.
| } | ||
|
|
||
| function getEmployees(listElement) { | ||
| const itemsArray = Array.from(list.children); |
There was a problem hiding this comment.
Similar to the sortList function, this line should use the listElement parameter that was passed into the function instead of relying on the global list variable.
| sortList(); | ||
| getEmployees(); |
There was a problem hiding this comment.
Now that the sortList and getEmployees functions are defined to accept an argument, you need to pass the list variable when you call them. For example: sortList(list).
2 participants
No description provided.