Skip to content

Solution#1912

Open
aholubko wants to merge 3 commits intomate-academy:masterfrom
aholubko:develop
Open

Solution#1912
aholubko wants to merge 3 commits intomate-academy:masterfrom
aholubko:develop

Conversation

@aholubko
Copy link

Copy link

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 getEmployees returns objects where name is 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

  1. Rate AI review example

  2. Re-request AI review example

  3. Add AI review comments example


return items.map((li) => {
return {
name: li.textContent.trim(),

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using li.textContent.trim() will return the entire text content of the

  • (including child elements like position/salary/age). This can produce a 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.

  • }

    sortList(list);
    getEmployees(list);

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    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.

    Copy link

    @mateacademy-ai-mentor mateacademy-ai-mentor left a comment

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    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

    1. Rate AI review example

    2. Re-request AI review example

    3. Add AI review comments example

    const items = Array.from(ul.querySelectorAll('li'));

    return items.map((li) => ({
    name: li.textContent.trim(),

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    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.

    Copy link

    @mateacademy-ai-mentor mateacademy-ai-mentor left a comment

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    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

    1. Rate AI review example


    return {
    name: nameEl ? nameEl.textContent.trim() : li.textContent.trim(),
    position: positionEl ? positionEl.dataset.position : li.dataset.position,

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    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),

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    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),

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    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.

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Labels

    None yet

    Projects

    None yet

    Development

    Successfully merging this pull request may close these issues.

    2 participants