Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Great job on implementing the core logic for sorting and parsing the employee data! Your code works correctly, but it needs a few adjustments to meet all the task requirements. The sortList and getEmployees functions are required to accept a list parameter in their definitions, like sortList(list), but they currently rely on a global variable. Additionally, the task requires a dedicated helper function for converting the salary string to a number to avoid repeating that logic. Please create this helper and use it in both of your main functions. Once these changes are made, your solution will be complete!
✨ 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
| // write code here | ||
| const list = document.querySelector('ul'); | ||
|
|
||
| function sortList() { |
There was a problem hiding this comment.
According to the requirements, the sortList function should accept a list property as a parameter. It currently relies on the global list variable, which makes the function less reusable.
src/scripts/main.js
Outdated
| Number(b.dataset.salary.replace('$', '')) - | ||
| Number(a.dataset.salary.replace('$', '')), |
There was a problem hiding this comment.
The task requires creating a helper function for converting the salary string to a number. This would help avoid repeating the conversion logic here and in the getEmployees function.
src/scripts/main.js
Outdated
| employees.forEach((employee) => list.append(employee)); | ||
| } | ||
|
|
||
| function getEmployees() { |
There was a problem hiding this comment.
Similar to sortList, the requirements state that the getEmployees function should also accept a list property as a parameter.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Excellent progress on this task! You've done a great job implementing the helper function and refactoring your code to use parameters. There's just one small but important correction needed in the sortList function before this can be approved.
Currently, the function accepts listElement as a parameter but then uses the global list variable to append the sorted items. To make the function fully self-contained and reusable, it should operate only on the parameters it receives.
To fix this, please update the forEach loop inside sortList to append to the listElement parameter instead of the global list variable:
// Inside the sortList function
sortedEmployees.forEach((employee) => {
listElement.append(employee); // Use the listElement parameter here
});With that one change, your solution will be perfect. 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
| (a, b) => convertString(b.dataset.salary) - convertString(a.dataset.salary), | ||
| ); | ||
|
|
||
| employees.forEach((employee) => list.append(employee)); |
There was a problem hiding this comment.
This line correctly appends the sorted employees. However, it uses the global list variable instead of the listElement parameter that was passed to the function. To make the function more robust and reusable, it should operate on the listElement parameter it receives.
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 it perfectly meets all the requirements. You did a great job addressing the feedback from the previous review; the sortList function is now robust and correctly utilizes the parameter passed to it. The code is clean, well-structured, and functions exactly as expected. Keep up the fantastic work!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
2 participants
No description provided.