add task solution#1994
Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
The sorting functionality works correctly, but changes are needed because the code doesn't follow the explicit task requirements. The task requires three specific function implementations: a getEmployees function that returns an array of employee objects with the schema { name, position, salary, age }, a sortList function that takes a list parameter, and a separate helper function for converting salary strings to numbers. Currently, the code has inline logic instead of these required named functions. Additionally, both functions must be called as per step 4 of the requirements. Please refactor the code to use the required function structure, and also update the README demo link (replace <your_account> with your actual GitHub username).
✨ 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 listChildren = document.querySelectorAll('li'); | ||
| const ArrayListChildren = [...listChildren]; | ||
|
|
||
| ArrayListChildren.sort((a, b) => { |
There was a problem hiding this comment.
The code sorts the list inline but is missing the required sortList function. The task specifies creating a function that takes a list property and sorts list items by salary.
| }); | ||
|
|
||
| list.innerHTML = ''; | ||
| ArrayListChildren.forEach((li) => list.appendChild(li)); |
There was a problem hiding this comment.
Missing the required getEmployees function. This function must return an array of employee objects with the schema { name, position, salary, age }.
| const cleanA = a.dataset.salary.replace(/[$,]/g, ''); | ||
| const cleanB = b.dataset.salary.replace(/[$,]/g, ''); | ||
|
|
||
| const salaryA = Number(cleanA); |
There was a problem hiding this comment.
The task requires a helper function for number conversion (converting salary string to number). Currently the conversion is done inline within the sort logic.
| }); | ||
|
|
||
| list.innerHTML = ''; | ||
| ArrayListChildren.forEach((li) => list.appendChild(li)); |
There was a problem hiding this comment.
Missing function calls. After defining sortList and getEmployees, both functions need to be called as per the task requirements (step 4).
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
The provided response is not valid JSON. It contains markdown text and formatting that is not part of the expected JSON structure. The correct JSON should only include the 'decision' and 'reason' fields as defined in the schema, with 'decision' being either 'APPROVE' or 'REQUEST_CHANGES', and 'reason' being a string explaining the decision.
✨ 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
| return Number(cleanString); | ||
| } | ||
|
|
||
| function getEmployees() { |
There was a problem hiding this comment.
CHECKLIST ITEM #7 VIOLATION: The getEmployees function must take a list parameter as required by the task. Currently it queries ul li directly instead of using the parameter. Update the function signature to function getEmployees(list) and use list.querySelectorAll('li') instead.
| return { | ||
| element: li, | ||
| name: li.textContent.trim(), | ||
| position: li.dataset.position, | ||
| salary: li.dataset.salary, | ||
| age: Number(li.dataset.age), | ||
| }; |
There was a problem hiding this comment.
CHECKLIST ITEM #8 VIOLATION: The employee object schema must be exactly { name, position, salary, age } without the extra element property. Remove the element: li line from the returned object.
|
|
||
| const listElement = document.querySelector('ul'); | ||
|
|
||
| sortList(listElement); |
There was a problem hiding this comment.
CHECKLIST ITEM #9 VIOLATION: Both functions must be explicitly called. Currently only sortList is called at the end. Add a call to getEmployees(listElement) to store the employees array in a variable (e.g., const employees = getEmployees(listElement);).
2 participants
solution