completed the task by SerhiyShimko · Pull Request #1920 · mate-academy/js_task_parse_list_DOM

SerhiyShimko · 2026-02-18T16:03:14Z

No description provided.

mateacademy-ai-mentor

Review Summary

I am requesting changes because several checklist-required behaviors are not fully met and will likely cause tests to fail.

Critical issues that must be fixed:

In src/scripts/main.js, the salary parsing logic is duplicated inside the sort comparator; the task explicitly requires a reusable helper like parseSalary(str) [CHECKLIST ITEM #17]. Please extract the parsing into parseSalary, and use it both in sortList and in getEmployees [CHECKLIST ITEM #16].
getEmployees currently pushes salary (and age) as raw strings from dataset; salary must be converted to a number using the helper, and tests may also expect age as a number [CHECKLIST ITEMS #3, #16, #19]. Also, name is taken from innerText, which can include extra text; instead, target only the element or text node that represents the employee’s name (or at least use a more precise selector) so the name field matches what tests expect.
sortList appends sorted items to a global perent variable, and the variable name is misspelled; the function should derive the parent from the passed list (for example list[0].parentElement) or accept a parent argument, and then append to that element rather than relying on a global [CHECKLIST ITEM #18].

Minor improvements (not blockers but recommended):

Prefer textContent.trim() over innerText for consistent text extraction.
After sorting, ensure getEmployees uses the elements in their new sorted order (e.g., by iterating the sorted array or the list’s children) so the returned employee array order matches the visible list.

You’re very close: the overall structure and logic are on the right track, and once you add the helper, fix the data types, and remove the global parent dependency, your solution should align well with the requirements.

✨ 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-02-18T16:07:12Z

src/scripts/main.js

+    const firstPersonSalary = firstPerson.dataset.salary
+      .slice(1)
+      .split(',')
+      .join('');
+    const secondPersonSalary = secondPerson.dataset.salary
+      .slice(1)
+      .split(',')
+      .join('');


The salary parsing logic is duplicated inside the comparator. The task explicitly requires writing a helper function to convert salary strings to numbers (checklist item #17). Extract this logic into a parseSalary(str) helper and use it both here and in getEmployees (checklist item #16).

mateacademy-ai-mentor · 2026-02-18T16:07:12Z

src/scripts/main.js

+  for (const result of resultList) {
+    perent.append(result);


sortList appends sorted items to the global perent variable rather than to the list passed as the function parameter. The checklist requires appending sorted items to the list (checklist item #18). Derive the container from the passed list (for example list[0].parentElement) or accept the parent as an argument instead of using the global variable.

mateacademy-ai-mentor · 2026-02-18T16:07:12Z

src/scripts/main.js


-// write code here
+const li = document.querySelectorAll('li');
+const perent = document.querySelector('ul');


The perent variable name appears to be a typo. Rename it to parent or, better, avoid the global and get the parent from the passed list for clarity and correctness.

mateacademy-ai-mentor · 2026-02-18T16:07:12Z

src/scripts/main.js

+    arraysPersons.push({
+      name: oneLi.innerText,
+      position: oneLi.dataset.position,
+      salary: oneLi.dataset.salary,
+      age: oneLi.dataset.age,


getEmployees pushes salary (and age) from dataset as raw strings. Per checklist items #16 and #19 the salary must be converted to a number (use the helper from checklist #17). Also name is taken from innerText which may include other text — ensure you extract only the employee name as tests expect.

mateacademy-ai-mentor

Review Summary

I am requesting changes because there are a few critical behaviors that don’t yet meet the task requirements and will likely cause tests to fail.

Critical issues that must be fixed:

parseSalary currently returns a string and callers then wrap it in Number(...). The requirement is that parseSalary itself returns a Number (strip the currency symbol, remove thousands separators, normalize decimal comma, then return Number(...)), and the comparator should subtract those numeric results directly.
In getEmployees, salary and age are kept as strings, and name is taken from li.textContent. Tests expect salary (from parseSalary) and age to be numbers, and name to come from the specific element that holds the name (e.g. oneLi.querySelector('.name')?.textContent.trim()), so update the extraction and conversions accordingly.
sortList uses list[0].parentNode without guarding for an empty list, and then you call getEmployees(li) with the original NodeList. Derive the parent from the passed list defensively (e.g. compute parent and return early if it’s missing), and either pass the sorted array returned from sortList into getEmployees or have getEmployees read from the list’s parent after sorting so the employee array order matches the visible DOM order.

Minor/clean-up suggestions:

Use textContent.trim() consistently when reading strings from the DOM.
Prefer parentElement over parentNode when dealing with elements.

You have the main structure and helper function in place; once you adjust the types, name selection, and ordering to match these requirements, your solution should align well with the expected behavior.

✨ 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-02-18T16:29:08Z

src/scripts/main.js

+    const firstPersonSalary = parseSalary(firstPerson.dataset.salary);
+    const secondPersonSalary = parseSalary(secondPerson.dataset.salary);
+
+    return Number(secondPersonSalary) - Number(firstPersonSalary);


The comparator converts parsed salaries with Number(...) while parseSalary currently returns a string. Per requirements, make parseSalary return a Number so parsing logic is not duplicated and the comparator can subtract numeric values directly.

mateacademy-ai-mentor · 2026-02-18T16:29:08Z

src/scripts/main.js

+
+function sortList(list) {
+  const resultList = Array.from(list).sort((firstPerson, secondPerson) => {
+    const firstPersonSalary = parseSalary(firstPerson.dataset.salary);


You're calling parseSalary and then wrapping results in Number(...). Move numeric conversion into parseSalary (return a Number) and remove the extra Number(...) calls in the comparator to satisfy the helper requirement and avoid duplicated logic.

mateacademy-ai-mentor · 2026-02-18T16:29:08Z

src/scripts/main.js

+  });
+
+  for (const result of resultList) {
+    list[0].parentNode.append(result);


Derive the parent from the passed list instead of using list[0].parentNode directly. Also guard against an empty list (e.g. const parent = list[0] && list[0].parentElement; if (!parent) return resultList;) to avoid runtime errors.

mateacademy-ai-mentor · 2026-02-18T16:29:08Z

src/scripts/main.js

+
+  for (const oneLi of list) {
+    arraysPersons.push({
+      name: oneLi.textContent,


Using oneLi.textContent for name can include other text from the li. Select the specific element that contains the name (e.g. oneLi.querySelector('.name')?.textContent.trim()) and use textContent.trim() so name matches expected test values.

mateacademy-ai-mentor · 2026-02-18T16:29:08Z

src/scripts/main.js

+      name: oneLi.textContent,
+      position: oneLi.dataset.position,
+      salary: parseSalary(oneLi.dataset.salary),
+      age: oneLi.dataset.age,


age is taken as a raw string from dataset. Convert it to a number (for example age: Number(oneLi.dataset.age)) because tests expect numeric types for salary and likely age as well.

mateacademy-ai-mentor · 2026-02-18T16:29:08Z

src/scripts/main.js

+sortList(li);
+getEmployees(li);


You call sortList(li) then getEmployees(li). querySelectorAll returns a static NodeList — pass the sorted array returned from sortList to getEmployees (for example const sorted = sortList(li); getEmployees(sorted);) or make getEmployees read elements from the list parent after sorting so the returned order matches the visible list.

mateacademy-ai-mentor · 2026-02-18T16:29:09Z

src/scripts/main.js

+getEmployees(li);
+
+function parseSalary(str) {
+  return str.slice(1).split(',').join('.');


parseSalary currently returns a modified string. Update it to return a Number (strip currency symbol, remove thousand separators, replace comma with dot if needed, then return Number(...)) so the helper fully satisfies the requirement to convert salary to a number and can be reused across functions.

mateacademy-ai-mentor

Review Summary

I am approving your solution as an educational override so you can keep progressing while you refine these concepts. You implemented all three required functions (sortList, getEmployees, parseSalary) and correctly used data-* attributes for salary, which covers several core requirements. The main things to improve are: 1) derive the parent element inside sortList from the passed list (e.g. const parent = list[0]?.parentElement) and return early if it’s missing, instead of relying on a top-level father variable [CHECKLIST ITEM #14, #25]; 2) extract the employee name from the dedicated .name element and use textContent.trim() so the object matches the expected shape [CHECKLIST ITEM #5, #24]; and 3) fix parseSalary so it removes currency symbols and thousand separators without turning "1,200" into 1.2, and always returns a Number [CHECKLIST ITEM #15, #16]. Also, call sortList only once, store the result in a variable, and pass that to getEmployees so the array order matches the sorted DOM [CHECKLIST ITEM #18]. Overall, you’re very close: the structure is right, and with these numeric parsing and DOM-handling tweaks, your solution will be both robust and test-friendly.

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

Rate AI review example ↩

mateacademy-ai-mentor · 2026-02-18T16:52:15Z

src/scripts/main.js


-// write code here
+const li = document.querySelectorAll('li');
+const father = li[0].parentNode;


Deriving father from li[0] at module scope is unsafe when the NodeList might be empty and also couples code to a global variable. Compute the parent element inside sortList from the passed list (e.g. const parent = list[0]?.parentElement) and return early if missing. This addresses checklist items #11 and #12.

mateacademy-ai-mentor · 2026-02-18T16:52:16Z

src/scripts/main.js

+const li = document.querySelectorAll('li');
+const father = li[0].parentNode;
+
+function sortList(list) {


sortList should not rely on the top-level father variable. Use a parent derived from the list argument (use parentElement) and append sorted items to that parent. Making sortList self-contained ensures it works with any list passed in (checklist items #12 and #14).

mateacademy-ai-mentor · 2026-02-18T16:52:16Z

src/scripts/main.js

+  const resultList = Array.from(list).sort((firstPerson, secondPerson) => {
+    const firstPersonSalary = parseSalary(firstPerson.dataset.salary, 'salary');
+    const secondPersonSalary = parseSalary(
+      secondPerson.dataset.salary,
+      'salary',
+    );
+
+    return secondPersonSalary - firstPersonSalary;


The comparator calls parseSalary(...) which must return numeric values. Ensure parseSalary reliably returns a Number (see the comment on line 46). Also make the comparator operate on Numbers directly (subtract numeric results) — that is already attempted but depends on correct parseSalary behavior (checklist items #15 and #16).

mateacademy-ai-mentor · 2026-02-18T16:52:16Z

src/scripts/main.js

+  for (const result of resultList) {
+    if (father) {
+      father.append(result);


Appending using the top-level father inside the loop keeps global coupling. Use a parent local to sortList and append there. Also ensure sortList is not called multiple times (see lines 41-42) to avoid unnecessary DOM operations and ordering issues (checklist items #14 and #18).

mateacademy-ai-mentor · 2026-02-18T16:52:16Z

src/scripts/main.js

+
+  for (const oneLi of list) {
+    arraysPersons.push({
+      name: oneLi.textContent,


Using oneLi.textContent for name will include other text (position/age) and may not match tests. Extract the name from the specific child (for example oneLi.querySelector('.name')?.textContent.trim()) and trim it. Tests expect name from the dedicated element and clean string (checklist item #5).

mateacademy-ai-mentor · 2026-02-18T16:52:16Z

src/scripts/main.js

+      salary: parseSalary(oneLi.dataset.salary, 'salary'),
+      age: parseSalary(oneLi.dataset.age, 'age'),


salary and age should be Numbers in the returned objects. You already call parseSalary, but ensure parseSalary returns Number for salary and age so getEmployees returns numeric salary and age (checklist items #5 and #16).

mateacademy-ai-mentor · 2026-02-18T16:52:16Z

src/scripts/main.js

+sortList(li);
+getEmployees(sortList(li));


Calling sortList(li); and then getEmployees(sortList(li)); runs sortList twice. Call sortList once, save its result (const sorted = sortList(li);) and then call getEmployees(sorted) (or call getEmployees on the parent’s children after sorting). This prevents duplicate DOM moves and guarantees the employee array order matches the visible list (checklist item #18 and previous-review suggestion #25).

mateacademy-ai-mentor · 2026-02-18T16:52:16Z

src/scripts/main.js

+
+function parseSalary(str, what) {
+  if (what === 'salary') {
+    return Number(str.slice(1).split(',').join('.'));


Current salary normalization (str.slice(1).split(',').join('.')) converts thousand separators into a decimal point (e.g. "1,200" -> "1.200" -> 1.2), which is incorrect. Instead remove currency symbols and thousands separators, and normalize a decimal comma if present, then return Number(...). For example: const s = str.replace(/[^0-9.,-]/g, ''); const normalized = s.replace(/,/g, ''); return Number(normalized); — or implement safer logic to preserve decimal part. Ensure parseSalary always returns a Number (checklist items #15 and #16).

completed the task

e2dcff7

mateacademy-ai-mentor suggested changes Feb 18, 2026

View reviewed changes

completed the task

3d238e3

SerhiyShimko requested a review from mateacademy-ai-mentor February 18, 2026 16:25

mateacademy-ai-mentor suggested changes Feb 18, 2026

View reviewed changes

completed the task

328046b

SerhiyShimko requested a review from mateacademy-ai-mentor February 18, 2026 16:47

mateacademy-ai-mentor approved these changes Feb 18, 2026

View reviewed changes

		salary: parseSalary(oneLi.dataset.salary, 'salary'),
		age: parseSalary(oneLi.dataset.age, 'age'),

		sortList(li);
		getEmployees(li);

		sortList(li);
		getEmployees(sortList(li));

Conversation

SerhiyShimko commented Feb 18, 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!

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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants