added solution by ParamooreXO · Pull Request #2024 · mate-academy/js_task_parse_list_DOM

ParamooreXO · 2026-07-09T10:11:40Z

DEMO LINK

mateacademy-ai-mentor

Review Summary

I am requesting changes because several core requirements of the task are not yet implemented, and this is causing functional test failures. The required functions sortList(list) and getEmployees(list) are missing, and your current logic works only with a global employees array instead of receiving and operating on a specific list element from the DOM. As a result, the <li> elements in the list are never reordered in the DOM, so the list is not actually sorted by salary in descending order, which directly breaks the Cypress test that checks the sorted list. In addition, salary values are kept as strings and converted inline with regex instead of using a dedicated helper function that returns a numeric salary, which the task explicitly asks for. To fix this, you should: (1) select the list element from the document, (2) implement sortList(list) to collect its <li> children, sort them by numeric salary using a helper like parseSalary(value), and append them back to the same list in sorted order, and (3) implement getEmployees(list) to return an array of { name, position, salary, age } with salary as a number. Once these functions are defined, called with the list element, and the DOM nodes are reordered rather than just rewriting dataset and text content, your solution will align with the requirements and should pass the failing test. You did a good job using dataset and building employee objects with the right keys, so reusing that approach inside the new functions will help you complete the task correctly.

✨ Please rate this review in the Platform's notification center¹. 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 button² 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 tag³.
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.

mateacademy-ai-mentor · 2026-07-09T10:14:33Z

 'use strict';

-// write code here
+const employees = [];


The task requires you to obtain the list element from the document and then pass it into the required functions; here you are only creating a global employees array and not working with a specific list node as described.

mateacademy-ai-mentor · 2026-07-09T10:14:33Z

-// write code here
+const employees = [];
+
+document.querySelectorAll('[data-salary]').forEach((listItem) => {


Instead of directly iterating all [data-salary] elements, the specification asks for sortList(list) and getEmployees(list) functions, so this logic should be moved into those functions and operate on the list argument.

mateacademy-ai-mentor · 2026-07-09T10:14:33Z

+    name: listItem.textContent.trim(),
+    position: listItem.dataset.position,
+    salary: listItem.dataset.salary,


According to the requirements, salary should be converted to a number via a helper function; here you keep the salary as a string taken directly from dataset.salary.

mateacademy-ai-mentor · 2026-07-09T10:14:33Z

+  });
+});
+
+employees.sort((a, b) => {


The sorting logic must be encapsulated in a sortList(list) function that works on the list’s items, not on a separate employees array; also, the salary-to-number conversion should be done in a helper instead of inline regex replacement.

mateacademy-ai-mentor · 2026-07-09T10:14:33Z

+});
+
+employees.sort((a, b) => {
+  return b.salary.replace(/[^0-9]/g, '') - a.salary.replace(/[^0-9]/g, '');


The sort comparator uses replace on b.salary twice and never on a.salary, which looks like a copy‑paste mistake and will not correctly compute the difference between b and a salaries.

mateacademy-ai-mentor · 2026-07-09T10:14:33Z

+  return b.salary.replace(/[^0-9]/g, '') - a.salary.replace(/[^0-9]/g, '');
+});
+
+document.querySelectorAll('li[data-salary]').forEach((listItem, index) => {


Rewriting dataset values and text content does not change the order of <li> elements, but the requirement is to sort the list by salary by reordering the list items in the DOM and appending them back to the list.

mateacademy-ai-mentor · 2026-07-09T10:14:33Z

+
+document.querySelectorAll('li[data-salary]').forEach((listItem, index) => {
+  listItem.dataset.position = `${employees[index].position}`;
+  listItem.dataset.salary = `$${employees[index].salary}`;


Adding a $ prefix when writing back dataset.salary changes the format of the data attribute in a way not requested; the attribute should remain a clean data value, while formatting belongs in display, not the dataset.

mateacademy-ai-mentor

Review Summary

I am requesting changes because there is still a critical mismatch with the task’s explicit requirements around salary handling and the helper function. The instructions require a dedicated helper (e.g., parseSalary(value)) to convert the salary data attribute from string to number, but right now the conversion is done inline with regex in sortList (around line 7) and duplicated again for bValue, which violates the requirement for a reusable helper. Additionally, in getEmployees (later in src/scripts/main.js), the salary field of the returned employee objects is kept as the original dataset.salary string, while the spec clearly states that salary must be numeric. These issues are critical because they directly break checklist items that define the expected API shape of getEmployees and the mandatory helper function.

To fix this, introduce a small helper like function parseSalary(value) { /* convert "50 000 ₽" to 50000 */ } once at the top of main.js, and then use it both in sortList for comparing salaries and in getEmployees when building each employee object. Make sure that after calling parseSalary, employee.salary is a number (e.g., 50000, not '50 000 ₽'), and remove any duplicated inline regex logic so all salary parsing goes through that helper. As a minor improvement, you could have getEmployees accept the <ul> element and access .children inside, aligning more closely with the original function signature, but this is optional compared to the salary issues. Once you add the helper and ensure numeric salaries everywhere, your solution will fully meet the functional and checklist requirements, and you’re otherwise very close to a complete, correct implementation.

✨ Please rate this review in the Platform's notification center¹. 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 button² 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 tag³.
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.

mateacademy-ai-mentor · 2026-07-09T10:51:30Z

+
+function sortList(li) {
+  li.sort((a, b) => {
+    const aValue = a.dataset.salary.replace(/[^0-9]/g, '');


The task explicitly requires a helper function to convert salary from the data attribute string to a number (checklist items #16, #17, and previous review #26). Here salary is converted inline, so consider extracting this logic into a dedicated helper like parseSalary(value) and reusing it.

mateacademy-ai-mentor · 2026-07-09T10:51:31Z

+function sortList(li) {
+  li.sort((a, b) => {
+    const aValue = a.dataset.salary.replace(/[^0-9]/g, '');
+    const bValue = b.dataset.salary.replace(/[^0-9]/g, '');


Similarly to line 7, this inline conversion for bValue should be handled by the same helper function that parses the salary string to a number, instead of duplicating the regex here.

mateacademy-ai-mentor · 2026-07-09T10:51:31Z

+function getEmployees(li) {
+  return li.map((item) => {
+    const position = item.dataset.position;
+    const salary = item.dataset.salary;


In getEmployees, salary is taken directly from dataset.salary and kept as a string, but the requirements specify that the employee object’s salary must be a number. Use the same salary-parsing helper here so salary is numeric in the returned objects.

mateacademy-ai-mentor

Review Summary

Your solution is approved. You correctly locate the <ul> element, build an array from its <li> children, then use sortList(list) to sort employees by salary in descending order and getEmployees(list) to construct objects with { name, position, salary, age }, which matches the task requirements. The dedicated parseSalary(value) helper cleanly handles salary conversion and is reused in both main functions, which is a nice improvement in terms of code reuse and clarity.

A small improvement opportunity is to make the numeric intent of parseSalary more explicit by returning Number(cleanedDigits) or parseInt(cleanedDigits, 10) instead of relying on implicit coercion in arithmetic operations; this will make your code more self-explanatory to readers. Overall, your implementation is focused, readable, and stays within the required scope of changing only main.js. Well done on addressing the previous review points and delivering a solid, maintainable solution.

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

Rate AI review example ↩

added solution

17bfbf5

mateacademy-ai-mentor suggested changes Jul 9, 2026

View reviewed changes

new version

6df67ff

ParamooreXO requested a review from mateacademy-ai-mentor July 9, 2026 10:50

mateacademy-ai-mentor suggested changes Jul 9, 2026

View reviewed changes

added helping function

7801ff2

ParamooreXO requested a review from mateacademy-ai-mentor July 9, 2026 11:16

mateacademy-ai-mentor approved these changes Jul 9, 2026

View reviewed changes

Uh oh!

Conversation

ParamooreXO commented Jul 9, 2026

Uh oh!

mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

Review Summary

Footnotes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

Review Summary

Footnotes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

Review Summary

Footnotes

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants