added new clean solution. I did it!!!!#1922
added new clean solution. I did it!!!!#1922TiFAu wants to merge 3 commits intomate-academy:masterfrom
Conversation
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am requesting changes because some core task requirements are not yet correctly implemented, which will likely break expected functionality and tests. You did implement sortList(list) and the salary string→number helper, but in src/scripts/main.js you currently select all li elements globally instead of targeting the specific employees list container, so the script may reorder unintended items. In your sorting logic you use .map only for side effects when appending nodes; this should be changed to .forEach (or a simple loop) and you should append explicitly to the list container (e.g. employeesList.append(child)), not via person.parentNode.append(person). The getEmployees(list) function does not meet the required schema: it leaves salary and age as strings and takes name from innerText, which will mix name, position, and salary; instead, extract name from the dedicated name element, use convertStringToNumber(person.dataset.salary) for salary, and Number(person.dataset.age) for age. Finally, you call getEmployees(listOfEmployees) but do not store or expose its returned array; assign it to a variable (e.g. const employees = getEmployees(listOfEmployees);) so it can be used or tested. Once you address these points, your solution should fully align with the assignment requirements and be in great shape.
✨ 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 | ||
| const listOfEmployees = [...document.querySelectorAll('li')]; |
There was a problem hiding this comment.
You select all
document.querySelectorAll('li'). The task asks to "Find and get a list from the document" — prefer selecting the specific list container (e.g. a specific ul by class or id) and then use its child
src/scripts/main.js
Outdated
| convertStringToNumber(person2.dataset.salary) - | ||
| convertStringToNumber(person1.dataset.salary), | ||
| ) | ||
| .map((person) => person.parentNode.append(person)); |
There was a problem hiding this comment.
.map is used here for side effects (appending nodes). .map is meant to transform arrays; use .forEach or a normal loop for side effects, and append to the list container explicitly (e.g. listContainer.append(child)) instead of person.parentNode.append(person) for clarity.
src/scripts/main.js
Outdated
| name: person.innerText, | ||
| position: person.dataset.position, | ||
| salary: person.dataset.salary, | ||
| age: person.dataset.age, |
There was a problem hiding this comment.
getEmployees uses person.innerText for name and leaves salary and age as dataset strings. This violates the schema and the technical constraint that salary must be converted to a Number. Use the helper convertStringToNumber(person.dataset.salary) for salary, convert age with Number(person.dataset.age) if a number is expected, and extract name from the specific child element that contains the name (e.g. person.querySelector('.name').innerText) instead of innerText which concatenates all nested text.
src/scripts/main.js
Outdated
| } | ||
|
|
||
| sortList(listOfEmployees); | ||
| getEmployees(listOfEmployees); |
There was a problem hiding this comment.
You call getEmployees(listOfEmployees) but don't store or use its returned array. If tests expect the array or other code depends on it, save it (e.g. const employees = getEmployees(listOfEmployees);) or export it as required by the project setup.
…for apppend . I did it!!!!
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am requesting changes because there are still a few critical issues that break the required functionality and schema. First, collecting list items via const listOfEmployees = [...targetList?.querySelectorAll('li')] can throw when targetList is undefined; you need a safe fallback like const listOfEmployees = targetList ? [...targetList.querySelectorAll('li')] : [] so the script doesn’t crash if the UL is missing. Second, sortList(list) currently sorts the passed list but appends items back into the module-level targetList, which couples the function to a global and means it won’t correctly sort an arbitrary list argument; instead, derive the container from the passed nodes (e.g. const container = list[0]?.closest('ul')) or accept the UL element and append to that. Third, getEmployees(list) does not yet build the employee objects in the required format: name is taken from person.innerText, position is converted to a number, and salary is left as a string. You should:
- read
namefrom the specific name element inside eachli(e.g.person.querySelector('.employee-name').innerTextor the correct selector), - keep
positionas a string from the correct child/dataset (no numeric conversion), - convert
salaryto aNumberusingconvertStringToNumber(person.dataset.salary), and - ensure
ageremains aNumberfrom the dataset.
Once these issues are fixed, your salary parsing helper and sort comparator logic will align well with the task requirements, and the solution should fully pass the tests.
✨ 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
| targetList = targetList.nextElementSibling; | ||
| } | ||
|
|
||
| const listOfEmployees = [...targetList?.querySelectorAll('li')]; |
There was a problem hiding this comment.
Spreading an optional result will throw if targetList is undefined. Use a safe fallback like targetList ? [...targetList.querySelectorAll('li')] : [] so the script doesn't crash when the expected list isn't present.
src/scripts/main.js
Outdated
| function sortList(list) { | ||
| list | ||
| .sort( | ||
| (person1, person2) => | ||
| convertStringToNumber(person2.dataset.salary) - | ||
| convertStringToNumber(person1.dataset.salary), | ||
| ) | ||
| .forEach((person) => targetList.append(person)); |
There was a problem hiding this comment.
sortList sorts the passed list but appends sorted items to the module-level targetList. This couples the function to a global variable — instead append to the actual container for the passed list (e.g. derive const container = list[0]?.closest('ul') or accept the UL element) so sorting works on any provided list.
| */ | ||
| function getEmployees(list) { | ||
| return list.map((person) => ({ | ||
| name: person.innerText, |
There was a problem hiding this comment.
name is taken from person.innerText, which mixes name/position/salary. Extract the name from the dedicated name element inside the LI (use the specific selector for the name element) to match the required schema.
src/scripts/main.js
Outdated
| function getEmployees(list) { | ||
| return list.map((person) => ({ | ||
| name: person.innerText, | ||
| position: convertStringToNumber(person.dataset.position), |
There was a problem hiding this comment.
position is being converted with convertStringToNumber, but position should be a string (e.g. 'Software Engineer'). Remove numeric conversion and read the position text from the appropriate child element or dataset string.
src/scripts/main.js
Outdated
| return list.map((person) => ({ | ||
| name: person.innerText, | ||
| position: convertStringToNumber(person.dataset.position), | ||
| salary: person.dataset.salary, |
There was a problem hiding this comment.
salary is returned as the raw dataset string. Convert it to Number using convertStringToNumber(person.dataset.salary) so the salary field in the returned objects is a Number as required by the task.
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am approving your solution as an educational override so you can keep progressing while you learn.
You did a solid job overall: you correctly locate the list element, use a helper to convert salary strings to numbers, sort by the data-salary attribute in descending order, and build employee objects using dataset for position, salary, and age. There are two important improvements to make next: in sortList (in src/scripts/main.js), the expression list[0]?.closest('ul').append(person) is unsafe if list is empty—first store the container in a variable (e.g. const container = list[0]?.closest('ul'); if (!container) return;) and then append to that. Also in getEmployees, instead of using person.innerText for name, read the name from the dedicated name element inside each <li> (for example, person.querySelector('.employee-name').innerText or whatever selector your HTML uses) so that only the exact name text is captured. These changes will make your code more robust and aligned with the requirements while preserving the good structure you already have. Keep going—your current solution shows a strong grasp of the core concepts, and these fixes are a natural next step in refining your implementation.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| convertStringToNumber(person2.dataset.salary) - | ||
| convertStringToNumber(person1.dataset.salary), | ||
| ) | ||
| .forEach((person) => list[0]?.closest('ul').append(person)); |
There was a problem hiding this comment.
This append expression is unsafe: list[0]?.closest('ul').append(person). Optional chaining is applied to list[0] but .append(...) is called on the result without guarding it, so if list is empty this becomes undefined.append(...) and throws. Derive and guard the container first, for example:
const container = list[0]?.closest('ul');
if (!container) return;
list.sort(...).forEach(person => container.append(person));This ensures the function works for any passed list and satisfies the requirement to append sorted items to the list container.
| */ | ||
| function getEmployees(list) { | ||
| return list.map((person) => ({ | ||
| name: person.innerText, |
There was a problem hiding this comment.
getEmployees must take the name from the specific name element inside each <li> (not from person.innerText). Using innerText can include other node text and will fail tests that expect the exact name field. Update to read the correct child element (use the selector that exists in your HTML, e.g. person.querySelector('.employee-name').innerText or the actual class/id used in the markup). position, salary (converted with your helper) and age (Number) are handled correctly.
No description provided.