solved #1896
solved #1896
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 sorting logic! The list is correctly sorted by salary. However, a few changes are needed to meet all the task requirements.
Here are the main points to address:
- Incorrect Employee Object Schema: The
getEmployeesfunction should return objects withname,position,salary, andage. Currently,positionandageare missing. You can get these values from thedata-positionanddata-ageattributes on the list items. - Incomplete
sortListFunction: The requirement is for thesortListfunction to also handle appending the sorted items back to the list. Please move the appending logic from the global scope into this function. - Missing Helper Function: The task requires a helper function to convert the salary string to a number to avoid repeating code. Please create this function and use it where the conversion is needed.
Once these are addressed, 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
| 'use strict'; | ||
|
|
||
| // write code here | ||
| // write code herefunction getEmployees(list) { |
There was a problem hiding this comment.
It looks like a leftover comment from the template is merged with the function declaration. This should be cleaned up to avoid potential syntax errors.
| return list.map((li) => ({ | ||
| name: li.textContent.trim(), | ||
| salary: Number(li.dataset.salary.replace(/[^\d]/g, '')), | ||
| element: li, | ||
| })); |
There was a problem hiding this comment.
The returned object does not match the required schema: { name, position, salary, age }. It's missing the position and age properties and includes an extra element property. You'll need to extract position and age from the list item's data attributes as well.
| })); | ||
| } | ||
|
|
||
| function sortList(list) { |
There was a problem hiding this comment.
While the sorting logic is correct, the task description requires the sortList function to also "Append sorted items to list". Currently, the appending logic is handled in the global scope (lines 29-31). This logic should be moved inside this function to meet the requirement.
src/scripts/main.js
Outdated
| Number(b.dataset.salary.replace(/[^\d]/g, '')) - | ||
| Number(a.dataset.salary.replace(/[^\d]/g, '')) |
There was a problem hiding this comment.
This logic for converting the salary from a string to a number is duplicated in the getEmployees function. The task description specifically asks to create a helper function for this conversion to avoid code repetition and improve maintainability.
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! The overall structure is much improved. However, a few changes are needed before this can be approved.
The main issue is that the list is not being sorted correctly. Inside your sort function, you are passing the entire <li> elements to parseSalary. Instead, you need to pass the salary string from each element's dataset, for example: parseSalary(b.dataset.salary) - parseSalary(a.dataset.salary).
Additionally, the getEmployees function should not add the element property to the employee objects, as the required schema is just { name, position, salary, age }. The sortList function should also not call getEmployees; its only job is to sort the DOM elements and append them.
Fixing these points will complete the task. 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
| name: li.textContent.trim(), | ||
| salary: parseSalary(li.dataset.salary), | ||
| position: li.dataset.position, | ||
| age: li.dataset.age, |
There was a problem hiding this comment.
For consistency, it would be better to convert the age to a number, just as you're doing with the salary. li.dataset.age returns a string, so you can wrap it with Number().
src/scripts/main.js
Outdated
| salary: parseSalary(li.dataset.salary), | ||
| position: li.dataset.position, | ||
| age: li.dataset.age, | ||
| element: li, |
There was a problem hiding this comment.
The required employee schema is { name, position, salary, age }. Adding the element property here violates this requirement. The getEmployees function should only be responsible for creating the data objects, not providing implementation details for other functions.
| function sortList(list) { | ||
| list.sort((a, b) => parseSalary(b) - parseSalary(a)); | ||
|
|
||
| const employees = getEmployees(list); | ||
| const ul = document.querySelector('ul'); | ||
|
|
||
| employees.forEach((person) => { | ||
| ul.append(person.element); | ||
| }); | ||
|
|
||
| return employees; | ||
| } |
There was a problem hiding this comment.
This function has mixed responsibilities. Its main purpose is to sort the DOM elements and append them. The call to getEmployees here is unnecessary for that task and couples the two functions too tightly.
After a correct list.sort() runs, the list array itself will contain the sorted <li> elements. You can then append these elements directly from the list array, which simplifies this function significantly.
src/scripts/main.js
Outdated
| } | ||
|
|
||
| function sortList(list) { | ||
| list.sort((a, b) => parseSalary(b) - parseSalary(a)); |
There was a problem hiding this comment.
The sorting logic is not correct here. The a and b variables are DOM <li> elements, but the parseSalary function expects a salary string. You need to pass the salary from each element's dataset to parseSalary, for example: parseSalary(b.dataset.salary).
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 submission! I'm happy to approve it. You've successfully addressed all the feedback from the previous review, and the code now works perfectly. The sorting logic is implemented correctly, the functions are well-structured with clear responsibilities, and the salary parsing is spot on. This is a fantastic improvement. 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.