Skip to content

solution#2021

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

solution#2021
Handsome010 wants to merge 3 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 Outdated
'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 Outdated

// 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 Outdated
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 Outdated
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.

@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 approving your solution as an educational override so you can keep progressing while you refine your JavaScript skills.

You’ve done a solid job implementing the main flow: selecting the list from the DOM, sorting by data-salary in descending order, and building an employees array from the list items—this shows good understanding of DOM traversal and array manipulation. The most important fix you still need is in getEmployees(list): you currently reuse a single obj outside the loop (around lines where you build res), which means every entry in the result array points to the same object; instead, create a new object inside the loop for each item before pushing it to res. Also, make sure salary is stored as a number, not a string—parse item.dataset.salary (e.g., by stripping non-digit characters and using Number(...)), ideally using a shared helper function that you can reuse in both sortList and getEmployees as the task suggests. Once those changes are made, each element in the returned array will be a distinct { name, position, salary, age } object with the correct types, and your implementation will fully match the requirements.

Overall, you’re very close and the structure of your solution is strong—fixing the object creation and salary parsing will turn this into a clean, correct implementation. Keep going; these are exactly the kinds of subtle issues that help you grow quickly as a developer.


✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨

Footnotes

  1. Rate AI review example

Comment thread src/scripts/main.js
Comment on lines +7 to +9
children.sort((a, b) => {
const salaryA = +a.dataset.salary.replace(/[^0-9]/g, '');
const salaryB = +b.dataset.salary.replace(/[^0-9]/g, '');

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

In sortList, this correctly sorts by data-salary in descending order and converts the salary string to a number; however, consider extracting this parsing logic into a helper function as suggested in the task description so it can be reused in getEmployees as well.

Comment thread src/scripts/main.js
return salaryB - salaryA;
});

children.forEach((item) => list.appendChild(item));

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Here you append sorted items to the global list variable instead of the l parameter passed to sortList. While this works with the current code (since l and list refer to the same element), it is more robust and in line with the function signature to use l.appendChild(item) so the function behaves correctly for any list passed in.

Comment thread src/scripts/main.js
Comment on lines +19 to +24
const obj = {
name: 0,
position: 0,
salary: 0,
age: 0,
};

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 define a single obj outside the loop and then mutate it for each child; this means every element in res will reference the same object, ending up with identical data (from the last item). You need to create a new employee object inside the loop for each item.

Comment thread src/scripts/main.js
Comment on lines +26 to +30
for (const item of children) {
obj.name = item.textContent.trim();
obj.position = item.dataset.position;
obj.salary = item.dataset.salary;
obj.age = item.dataset.age;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Inside the loop you’re setting obj.salary directly from item.dataset.salary, which is a string. The requirements state salary from the data attribute must be converted to a number (ideally using a helper function). Update this to parse the salary to a numeric value, similar to how you do in sortList.

Comment thread src/scripts/main.js
obj.salary = item.dataset.salary;
obj.age = item.dataset.age;

res.push(obj);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Because obj is reused, each res.push(obj) just adds another reference to the same object rather than a distinct employee entry. After you move object creation inside the loop, res.push will correctly add separate objects for each employee.

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