Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
This requires a few changes to meet the task requirements. You've made a good start by selecting the elements and implementing sorting logic, but there are three main issues to address: 1. Your code is currently in the global scope, but it needs to be placed inside the two required functions: sortList and getEmployees. 2. The list is sorted in ascending order, but the requirement is to sort in descending order (highest salary first). You can fix this by reversing the logic in your sort comparison. 3. The getEmployees function, which should parse the list and return an array of employee objects, is currently missing and needs to be implemented. Addressing these critical points will align your solution with 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 list = document.querySelector('ul'); | ||
| const items = [...document.querySelectorAll('li')]; | ||
|
|
||
| items.sort((a, b) => { | ||
| const getNumericSalary = (el) => { | ||
| return parseFloat(el.dataset.salary.replace(/[^0-9.-]+/g, '')); | ||
| }; | ||
|
|
||
| return getNumericSalary(a) - getNumericSalary(b); | ||
| }); | ||
|
|
||
| items.forEach((item) => list.appendChild(item)); |
There was a problem hiding this comment.
The task requires you to create two functions, sortList and getEmployees, and place your logic within them. Currently, the code is in the global scope.
src/scripts/main.js
Outdated
| return parseFloat(el.dataset.salary.replace(/[^0-9.-]+/g, '')); | ||
| }; | ||
|
|
||
| return getNumericSalary(a) - getNumericSalary(b); |
There was a problem hiding this comment.
This expression sorts the items in ascending order. The requirement is to sort them in descending order.
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 correctly implemented the sorting logic and structured the code well. However, a few changes are needed to meet all the requirements before this can be approved.
Here are the key things to address:
- Function Parameters: Both
getEmployeesandsortListmust be updated to accept alistas a parameter, as specified in the requirements. - Incorrect Object Schema: The
getEmployeesfunction needs to return objects that match the required{ name, position, salary, age }schema. You'll need to parse thename,position(from the text content), andage(from thedata-ageattribute) for each item. - Missing Helper Function: The salary parsing logic (line 10) should be extracted into its own separate helper function.
Addressing 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
| const getEmployees = () => { | ||
| const items = [...document.querySelectorAll('li[data-salary]')]; | ||
|
|
||
| return items.map((item) => ({ | ||
| element: item, | ||
| salary: parseFloat(item.dataset.salary.replace(/[^0-9.-]+/g, '')), | ||
| })); | ||
| }; |
There was a problem hiding this comment.
This function is on the right track, but it needs a few adjustments to meet all requirements:
- It should accept a
listas a parameter. - The returned objects should match the
{ name, position, salary, age }schema. You'll need to parse thename,position(from the text content), andage(from thedata-ageattribute) for each item. Theelementproperty is not part of the required schema. - The salary parsing logic on line 10 should be extracted into a separate helper function.
| const sortList = () => { | ||
| const list = document.querySelector('ul'); | ||
| const employees = getEmployees(); | ||
|
|
||
| employees.sort((a, b) => b.salary - a.salary); | ||
|
|
||
| employees.forEach((obj) => list.appendChild(obj.element)); | ||
| }; |
There was a problem hiding this comment.
This function also needs to accept a list as a parameter to meet the requirements.
Also, note that your current implementation relies on getEmployees returning DOM elements. Once you update getEmployees to return only data objects as required, this sorting logic will need to be revised. The sortList function should sort the <li> elements directly.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Sorry, the pull request review was unsuccessful for the following reasons:
Warning
No changes were made in the pull request since the last review. Please make sure that you have added files to the pull request and committed the changes.
[CODE: NCM]
If the issue is within your control, please address it.
The next review attempt will automatically launch in about 1 hour.
If you believe this is an error or need assistance, please contact the support team.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Sorry, the pull request review was unsuccessful for the following reasons:
Warning
No changes were made in the pull request since the last review. Please make sure that you have added files to the pull request and committed the changes.
[CODE: NCM]
If the issue is within your control, please address it.
The next review attempt will automatically launch in about 1 hour.
If you believe this is an error or need assistance, please contact the support team.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Sorry, the pull request review was unsuccessful for the following reasons:
Warning
No changes were made in the pull request since the last review. Please make sure that you have added files to the pull request and committed the changes.
[CODE: NCM]
If the issue is within your control, please address it.
The next review attempt will automatically launch in about 1 hour.
If you believe this is an error or need assistance, please contact the support team.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Sorry, the pull request review was unsuccessful for the following reasons:
Warning
No changes were made in the pull request since the last review. Please make sure that you have added files to the pull request and committed the changes.
[CODE: NCM]
If the issue is within your control, please address it.
The next review attempt will automatically launch in about 1 hour.
If you believe this is an error or need assistance, please contact the support team.
3 participants
No description provided.