Skip to content

Develop#1971

Open
Vitaligna wants to merge 4 commits into
mate-academy:masterfrom
Vitaligna:develop
Open

Develop#1971
Vitaligna wants to merge 4 commits into
mate-academy:masterfrom
Vitaligna:develop

Conversation

@Vitaligna
Copy link
Copy Markdown

Copy link
Copy Markdown

@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

This solution needs some changes before it can be accepted because one core data requirement and one explicit task constraint are currently not satisfied. In src/scripts/main.js, inside getEmployees, the name field is built from item.textContent.trim(), which pulls all text from the <li> (including position and other labels), so the resulting employee.name will not contain only the person's name as required and will likely cause the tests that validate the employee schema to fail. Please change this so that you read only the dedicated name element or attribute, for example by using something like item.querySelector('.name').textContent.trim() or a data-name attribute, so that name is strictly the employee’s name. In addition, the task explicitly says to modify only src/scripts/main.js, but your PR also changes other files (such as package.json, package-lock.json, README, or workflows), so those unrelated changes need to be reverted so that only main.js is updated. As a medium‑priority improvement, be aware that renderResult appends a <pre> element with JSON to document.body; you may want to remove or gate this debug output if the tests expect the DOM structure to remain unchanged. Overall, your salary parsing, sorting by dataset.salary in descending order, and numeric conversion for salary and age look good, so once you fix the name extraction and limit changes to main.js, the solution should be in good 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

  1. Rate AI review example

  2. Re-request AI review example

  3. Add AI review comments example

Comment thread src/scripts/main.js Outdated

function getEmployees(ulElement) {
return Array.from(ulElement.querySelectorAll('li')).map((item) => ({
name: item.textContent.trim(),
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The name field is taken from item.textContent.trim(), which returns all text within the

  • (including position and other text). This likely violates the required employee schema where name should contain only the person's name. Consider selecting the specific child element that contains the name (for example item.querySelector('.name').textContent.trim()) or using a dedicated data attribute for the name.

  • Copy link
    Copy Markdown

    @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, while your core logic (parseSalary, sortList, and getEmployees) meets the functional requirements, there are still a couple of critical issues that can break the task’s constraints and tests. First, the PR currently modifies files outside src/scripts/main.js (such as package.json, lockfiles, README, or workflows), which violates the explicit requirement that only main.js should be changed for this task; please revert all unrelated file changes so that only src/scripts/main.js is touched. Second, in src/scripts/main.js your renderResult function appends a <pre> element to document.body and is called unconditionally near the bottom of the file, which changes the DOM/layout even in tests and production; you should either remove this debug rendering or guard it so it only runs in a true debug/development mode and not during normal execution. Third, you query the list with const employeeList = document.querySelector('ul') and immediately pass it into sortList and getEmployees without checking for null, which can cause a runtime error if the expected <ul> is missing; add a simple guard like if (!employeeList) return; (or equivalent) before using it. Once these issues are addressed, your implementation should be well aligned with the requirements, and the existing data parsing and sorting logic looks solid.


    ✨ 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

    Comment thread src/scripts/main.js Outdated
    const pre = document.createElement('pre');

    pre.textContent = JSON.stringify(employees, null, 2);
    document.body.appendChild(pre);
    Copy link
    Copy Markdown

    Choose a reason for hiding this comment

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

    The function renderResult appends a <pre> element to document.body. The task explicitly says not to change styles or the HTML layout, and tests sometimes expect the DOM to remain unchanged. Consider removing this debug output or gating it behind a condition so tests/production won't modify the page structure.

    Comment thread src/scripts/main.js Outdated

    const employeeArray = getEmployees(employeeList);

    renderResult(employeeArray);
    Copy link
    Copy Markdown

    Choose a reason for hiding this comment

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

    You call renderResult(employeeArray) unconditionally. If you need to keep a debug render, guard it (e.g., only run in development) or remove the call before final submission so the DOM is not changed.

    Comment thread src/scripts/main.js Outdated
    Comment on lines +33 to +37
    const employeeList = document.querySelector('ul');

    sortList(employeeList);

    const employeeArray = getEmployees(employeeList);
    Copy link
    Copy Markdown

    Choose a reason for hiding this comment

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

    You query the list with document.querySelector('ul') and immediately pass it to sortList/getEmployees. Add a null check to ensure employeeList exists before using it to avoid runtime errors when the expected <ul> is not present.

    Copy link
    Copy Markdown

    @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 for educational purposes so you can continue progressing, but there are a couple of important issues you should fix next. You implemented the core requirements in src/scripts/main.js well: parseSalary, sortList(list), and getEmployees(list) all follow the spec, including sorting in descending order and returning objects with { name, position, salary, age }, and you correctly guard against a missing list element before calling these functions. A critical issue is that your pull request also modifies package.json, package-lock.json, README.md, and .github/workflows/test.yml-template even though the task requires changing only src/scripts/main.js; you should revert those extra changes so that only main.js is updated. Another critical problem is that the Cypress tests are failing because getEmployees is trying to read textContent from a null element (e.g., where you do something like someElement.textContent around the lines that parse each <li>), which means your selector doesn’t always find the node; add a null check before accessing textContent and verify your selectors match the actual DOM structure. For example, you can do:

    const salaryEl = li.querySelector('[data-salary]');
    if (!salaryEl) return; // or skip/handle this item
    const salary = parseSalary(salaryEl.dataset.salary);

    so that you never read from null. As a minor improvement, consider using the array returned from getEmployees(list) (e.g., logging or passing it to another function) so the result is clearly used, even though the task only requires that you call the function. Overall, your main logic is solid and close to production-ready; by tightening up the DOM safety checks and limiting the PR to the required file, you’ll have a clean, fully compliant solution.


    ✨ 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

    Copy link
    Copy Markdown

    @etojeDenys etojeDenys left a comment

    Choose a reason for hiding this comment

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

    good job

    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.

    3 participants