Repository navigation
solution - #2095
solution#2095MarcelloTar wants to merge 3 commits into
Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Your implementation has several critical issues that don't meet the task requirements. The main problems are: 1) sortList receives an array from getEmployees() instead of taking a DOM list parameter and sorting li elements directly by accessing dataset.salary; 2) The required helper function for salary string-to-number conversion is missing - it needs to be a standalone function; 3) sortList returns a sorted array but doesn't append the sorted items to the DOM list as explicitly required; 4) The functions aren't called separately after definition - they're chained together instead. These are blockers because they violate the explicit architectural requirements in the task description, even though the visual output may appear correct. Please refactor to match the specified implementation pattern.
✨ 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
|
|
||
| // console.log({...list[0].dataset}); | ||
|
|
||
| function sortList(list) { |
There was a problem hiding this comment.
The sortList function should take a DOM list (li elements) as parameter and sort them by extracting salary from data attributes, not receive an already-processed array. Per requirements: 'takes a list property, and sorts list items in descending order by salary from data attribute'
| function sortList(list) { | ||
| return [...list].sort((a, b) => { | ||
| return b.salary - a.salary; | ||
| }); |
There was a problem hiding this comment.
Missing helper function to convert salary string to number. The task explicitly requires: 'write helper function for it' to convert the salary string from data attribute to a number.
| function sortList(list) { | ||
| return [...list].sort((a, b) => { | ||
| return b.salary - a.salary; | ||
| }); |
There was a problem hiding this comment.
The sortList function should append sorted items to the list. Currently it only returns sorted items but doesn't append them to the DOM. The task states: 'Append sorted items to list'.
| function getEmployees(list) { | ||
| return [...list].map((item) => { | ||
| return { | ||
| ...item.dataset, | ||
| salary: +item.dataset.salary.slice(1).replaceAll(',', ''), | ||
| name: item.textContent, | ||
| }; | ||
| }); |
There was a problem hiding this comment.
The getEmployees function spreads all dataset properties which may include extra fields. The schema should only contain { name, position, salary, age }. Also, the function should take a DOM list parameter, not an array.
|
|
||
| sortList(getEmployees(lists)).forEach((item) => { | ||
| const li = document.createElement('li'); | ||
|
|
||
| li.setAttribute('data-position', item.position); | ||
| li.setAttribute('data-salary', `$${item.salary.toLocaleString('en-US')}`); | ||
| li.setAttribute('data-age', item.age); | ||
|
|
||
| li.textContent = item.name; | ||
|
|
||
| ul.append(li); |
There was a problem hiding this comment.
Both functions should be called separately after defining them. The current code mixes sorting and DOM manipulation together instead of having sortList handle the sorting and DOM update, then getEmployees return the employee array.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Your implementation has several critical issues that violate the task requirements:
-
sortList must sort DOM elements directly - Currently it receives a DOM list but immediately converts it to an array via
getEmployees(), instead of sortinglielements by accessingdataset.salarydirectly on each element. -
Missing required helper function - The task explicitly requires a standalone helper function to convert salary strings (like "$1,000") to numbers. This function should be used by
sortList. -
sortList must append to DOM, not return array - The requirement states "Append sorted items to list" - your function returns an array of HTML strings instead of manipulating the DOM directly.
-
Functions must be called separately - Both
getEmployeesandsortListneed to be called independently after their definitions, not nested within each other.
Please refactor your code to address these architectural issues while maintaining the same visual output.
✨ 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
|
|
||
| // console.log({...list[0].dataset}); | ||
|
|
||
| function sortList(list) { |
There was a problem hiding this comment.
sortList should receive a DOM list (like ul element) and sort li elements directly by accessing dataset.salary, not call getEmployees() to get an array. The task requires sorting DOM elements, not creating new HTML strings.
|
|
||
| // console.log({...list[0].dataset}); | ||
|
|
||
| function sortList(list) { |
There was a problem hiding this comment.
Missing required standalone helper function to convert salary string to number. The task explicitly requires a helper function for this conversion that sortList should use.
| ${item.name} | ||
| </li> | ||
| `; | ||
| }); |
There was a problem hiding this comment.
sortList should append sorted items to the DOM list using append or similar, not return an array of HTML strings. The task states: 'Append sorted items to list.'
|
|
||
| ul.innerHTML = ''; | ||
|
|
||
| sortList(lists).forEach((item) => { |
There was a problem hiding this comment.
getEmployees is being called inside sortList, but the task requires both functions to be called separately after their definitions, not chained or nested together.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Your solution is approved with an educational override to help you progress while learning. The core functionality works well - the sortList function correctly sorts by salary using data attributes, and the conversionToANumber helper properly converts salary strings. However, there's an important fix needed in getEmployees: the name property should come from item.dataset.name rather than item.textContent, since textContent would include all nested text within the <li> element. This is a common mistake when working with data attributes, and fixing it will make your extraction more accurate. Keep up the good work with the overall structure and helper function implementation!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| function getEmployees(list) { | ||
| return [...list].map((item) => { | ||
| return { | ||
| ...item.dataset, | ||
| salary: +item.dataset.salary.slice(1).replaceAll(',', ''), | ||
| name: item.textContent, |
There was a problem hiding this comment.
The name property should come from item.dataset.name, not item.textContent. Using textContent would include all text content from nested elements within the <li>, not just the name.
DEMO LINK