Skip to content

solution#2021

Open
Handsome010 wants to merge 2 commits into
mate-academy:masterfrom
Handsome010:develop
Open

solution#2021
Handsome010 wants to merge 2 commits into
mate-academy:masterfrom
Handsome010:develop

Conversation

@Handsome010

Copy link
Copy Markdown

No description provided.

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

Copy link
Copy Markdown

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 the core functionality required by the task is not implemented yet, and current tests that check sorting behavior are failing. In src/scripts/main.js, the sortList(list) function that should sort employees by salary in descending order using the data-salary attribute is missing, so the list cannot be sorted as required [CHECKLIST ITEM #2, #4, #7, #16]. The getEmployees(list) function that must build an array of employee objects with { name, position, salary, age } from the list items’ data attributes is also not present, so the employee collection requirement is not met [CHECKLIST ITEM #3, #5, #6, #8, #20]. Additionally, there is no helper or logic to parse salary strings (e.g., stripping non‑digits and converting to a number), which is necessary to perform correct numeric sorting [CHECKLIST ITEM #17, #18]. The DOM list element is never queried with something like document.querySelector('#employees-list'), and sortList / getEmployees are not invoked, so even once implemented, they would not run [CHECKLIST ITEM #9, #15, #21].

To fix this, implement sortList(list) so it: 1) reads each item’s dataset.salary, 2) converts it to a number, 3) sorts the items in descending order, and 4) appends the sorted items back into the list. Then implement getEmployees(list) to iterate over all list items, read data-name, data-position, data-salary, and data-age, convert salary (and age if needed) to numbers, and return an array of objects with those fields. At the top-level of main.js, select the list element from the DOM, call sortList(list), and then call getEmployees(list) (optionally logging the result) so the behavior is actually executed when the page loads. Finally, update the demo link placeholder <your_account> to your actual GitHub username in the README or description so the link requirement is satisfied. Once these are in place and Cypress tests pass (particularly the “should be sorted by salary DSC” test), your solution will be in good shape and aligned with the task’s expectations.


✨ 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

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

Copy link
Copy Markdown

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 the core required functionality is still missing from src/scripts/main.js, which blocks the task from being completed. The functions sortList(list) and getEmployees(list) are not implemented at all, so the employees list is not being sorted by salary and no array of { name, position, salary, age } objects is returned, which violates the main requirements and related checklist items. Instead, you currently collect text content and data-salary values into arrays and log them, but these arrays are unused and console.log also triggers an ESLint error. To fix this, you should: (1) select the specific employees list element (e.g., via document.querySelector('#employees-list') or similar), (2) implement sortList(list) to sort the li elements by numeric data-salary in descending order using a salary-parsing helper, and then append the sorted items back into the same list, and (3) implement getEmployees(list) to read data-name, data-position, data-salary, and data-age from each li and return an array of employee objects with the required schema. You also need to call both functions so they actually run when the script loads, and remove the unused variables (res, salaries as currently written) and the console.log to satisfy the linter. As a smaller improvement, remember to update the demo link placeholder <your_account> with your actual GitHub username in the README/description to meet that checklist requirement. Once these changes are made, your solution will align with the task goals and should allow the tests to pass; you’re close, you just need to wire the parsing and sorting into the specified functions and return the employees array properly.


✨ 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
'use strict';

// write code here
const res = [...document.querySelectorAll('li')].map((item) => 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 task requires implementing sortList(list) and getEmployees(list) that operate on the list element, but here you only build a res array of text content without defining or using those functions, so the main functionality is missing.

Comment thread src/scripts/main.js

// write code here
const res = [...document.querySelectorAll('li')].map((item) => item.textContent.trim());
const salaries = [];

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This array of salaries is collected but never used to sort the DOM list or build employee objects; you should instead use a helper to parse salary inside sortList and getEmployees as per the requirements.

Comment thread src/scripts/main.js
Comment on lines +6 to +7
for (const item of document.querySelectorAll('li')) {
salaries.push(+item.dataset.salary.slice(1).replaceAll(',', ''));

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Looping over all li elements and pushing parsed salaries is a good start for salary parsing, but the task requires using this parsing logic in a sortList(list) function to sort items in-place, not just accumulating into an unused array.

Comment thread src/scripts/main.js
for (const item of document.querySelectorAll('li')) {
salaries.push(+item.dataset.salary.slice(1).replaceAll(',', ''));
}
console.log(salaries);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

console.log(salaries) is not part of the required behavior; instead, you should return the employees array from getEmployees(list) and rely on tests rather than logging.

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