Conversation
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am requesting changes because there are a couple of requirement-level issues that likely break the expected behavior and tests. First, in src/scripts/main.js you derive name using something like li.textContent.trim(), which will include all the text inside the <li> (name, position, salary, age, etc.), not just the person’s name; the task expects name to be the name only, so you should instead read from the specific child element that holds the name (for example, li.querySelector('.name').textContent.trim() or whatever selector matches the name span in your HTML). Second, you call getEmployees(list); but you do not store or expose the returned array; the requirement is to “get an array of employees”, so you should assign this to a variable (e.g. const employees = getEmployees(list);) or otherwise make it accessible for tests/consumers.
Critical issues to fix:
- Ensure
getEmployeesreturns objects wherenameis only the employee’s name, not the full<li>text. - Capture the result of
getEmployees(list)in a variable or export so that the produced employees array can actually be used.
Once you adjust the name extraction and make the employees array available, the rest of your solution (salary parsing, numeric sorting, DOM re-append) looks solid and aligned with the task, so these changes should bring your implementation fully in line 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
|
|
||
| return items.map((li) => { | ||
| return { | ||
| name: li.textContent.trim(), |
There was a problem hiding this comment.
Using li.textContent.trim() will return the entire text content of the
name value that contains extra text. Consider selecting the specific element that contains the name (for example li.querySelector('.name').textContent.trim() or the exact element/class used in the markup) so name is the person’s name only.
src/scripts/main.js
Outdated
| } | ||
|
|
||
| sortList(list); | ||
| getEmployees(list); |
There was a problem hiding this comment.
You call getEmployees(list); but do not store or expose the returned array. The task asks to "Get an array of employees" — assign the result to a variable (for example const employees = getEmployees(list);) or otherwise make it available to tests/consumers.
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am requesting changes because one core requirement is still not correctly implemented and will cause the tests to fail. In src/scripts/main.js, your getEmployees function currently sets name using li.textContent.trim(), which includes the entire <li> text (name, position, salary, age) instead of just the employee’s name; the task explicitly requires the name field to contain only the person’s name. To fix this, update the name extraction to read from the specific child element that holds the name, for example:
const nameEl = li.querySelector('.name');
name: nameEl ? nameEl.textContent.trim() : li.textContent.trim(),Adjust the selector (e.g. .name) to match your actual HTML structure so only the name text is captured. Aside from this, your salary parsing, sorting logic, and exposure of the employees array on window look good and in line with the task. Once you correct the name extraction and ensure tests pass, your solution should fully meet 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 items = Array.from(ul.querySelectorAll('li')); | ||
|
|
||
| return items.map((li) => ({ | ||
| name: li.textContent.trim(), |
There was a problem hiding this comment.
getEmployees uses li.textContent.trim() for name, which will include the whole list item's text (name, position, salary, age). The task requires name to be only the employee's name. Replace this with reading the specific element that contains the name, for example:
const nameEl = li.querySelector('.name');
name: nameEl ? nameEl.textContent.trim() : li.textContent.trim(),Adjust the selector (.name) to match your HTML structure. This ensures the name field contains only the employee's name as required.
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 fix a few important issues in src/scripts/main.js. You did a solid job setting up getSalary, implementing sortList to sort the <li> elements by salary in descending order, and exposing employees on window, which matches the overall requirements well. However, in getEmployees there are a few critical bugs: salaryEl.getSalary(li) (around where you build the employee object) should use the standalone helper getSalary(li) instead of calling it as a method, ageEl.Number(...) should be replaced with Number(ageEl.textContent) (or Number(li.dataset.age)), and position should come from positionEl.textContent.trim() instead of dataset.position so you get the actual job title string [CHECKLIST ITEM #12]. Also double-check that name only uses the .name element’s text and not the full li.textContent, so it does not include position/salary/age in the name field [CHECKLIST ITEM #15]. Finally, remember to fix your demo link in the README/PR by replacing the placeholder with your GitHub username and removing any stray characters so it becomes a valid GitHub Pages URL. Overall, you’re very close—your structure and use of helpers are correct, and with a few careful fixes to how you read and convert the fields, your implementation will fully match the required { name, position, salary, age } schema. Keep going—you’re clearly understanding the main concepts, and these parsing details are a great chance to sharpen your DOM and type-conversion skills.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
|
|
||
| return { | ||
| name: nameEl ? nameEl.textContent.trim() : li.textContent.trim(), | ||
| position: positionEl ? positionEl.dataset.position : li.dataset.position, |
There was a problem hiding this comment.
positionEl ? positionEl.dataset.position : li.dataset.position assumes the position is stored in a data attribute. Usually the position is text inside the .position element. Use the element text (e.g. positionEl ? positionEl.textContent.trim() : li.dataset.position) so position contains the job title string required by the schema.
| return { | ||
| name: nameEl ? nameEl.textContent.trim() : li.textContent.trim(), | ||
| position: positionEl ? positionEl.dataset.position : li.dataset.position, | ||
| salary: salaryEl ? salaryEl.getSalary(li) : getSalary(li), |
There was a problem hiding this comment.
This calls salaryEl.getSalary(li) but getSalary is a standalone function, not a method on the element. This will throw. Use the helper getSalary(li) (or parse salaryEl.textContent and convert it) so salary is a Number, e.g. salary: getSalary(li).
| name: nameEl ? nameEl.textContent.trim() : li.textContent.trim(), | ||
| position: positionEl ? positionEl.dataset.position : li.dataset.position, | ||
| salary: salaryEl ? salaryEl.getSalary(li) : getSalary(li), | ||
| age: ageEl ? ageEl.Number(li.dataset.age) : Number(li.dataset.age), |
There was a problem hiding this comment.
ageEl.Number(...) is invalid (DOM elements have no Number method). Convert age to a number using Number(ageEl.textContent) (or Number(li.dataset.age)) so age is a Number in the returned object.
DEMO LINK