sorted list by radykhovska · Pull Request #1976 · mate-academy/js_task_parse_list_DOM

radykhovska · 2026-04-21T08:56:19Z

No description provided.

mateacademy-ai-mentor

Review Summary

I am requesting changes because some core checklist requirements are not yet implemented and current tests would fail. In src/scripts/main.js, the required getEmployees(list) function is completely missing, so there is no way to produce the array of employee objects with the { name, position, salary, age } schema as required by the task [CHECKLIST ITEM #3, #5, #6]. Your sortList function also ignores its elements parameter and instead sorts a globally captured list, and the DOM appending of the sorted <li> elements happens outside the function; this means the function does not match the expected API/behavior (it should both sort and append items within itself) [CHECKLIST ITEM #14, #16]. To fix this, update sortList(listElement) so it uses listElement to obtain its <li> children, sorts them by numeric salary (you can keep using parseToNumber), and appends the sorted items back into listElement inside the function before returning. Then implement getEmployees(listElement) to iterate over the same <li> elements, read the name and position text, convert salary and age (from text or data attributes) to numbers, build objects { name, position, salary, age }, and return the resulting array. Finally, make sure you actually call both sortList(ulElement) and getEmployees(ulElement) so the behavior is exercised and tests can locate these functions. Your salary parsing and basic sort logic are on the right track, so with these structural fixes you should be able to fully satisfy the task 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-04-21T08:58:08Z

+
+const parseToNumber = (string) => +string.replace(/[^0-9.-]+/g, '');
+
+const sortList = function (elements) {


The sortList function accepts an elements parameter but ignores it and uses the outer list variable instead. Use the passed elements to operate on the provided list (checklist #14).

mateacademy-ai-mentor · 2026-04-21T08:58:08Z

+
+const ul = document.querySelector('ul');
+
+sortedList.forEach((el) => ul.appendChild(el));


Step 2 and checklist #16 expect sortList to append the sorted items to the list. Here the append is done outside the function (line 17). Move the append logic into sortList (or ensure sortList's behavior matches the task description) so the function both sorts and updates the DOM.

mateacademy-ai-mentor · 2026-04-21T08:58:08Z

+  );
+};
+
+const sortedList = sortList(list);


The getEmployees function required by checklist #5 and checklist #3 is missing. Implement getEmployees(list) that returns an array of objects using the schema { name, position, salary, age }, and call it as required (checklist #6).

mateacademy-ai-mentor · 2026-04-21T08:58:08Z

+    (a, b) => parseToNumber(b.dataset.salary) - parseToNumber(a.dataset.salary),
+  );
+};
+
+const sortedList = sortList(list);


You call sortList(list) and then append elements manually; while sorting logic and parseToNumber are correct for converting salary strings to numbers, the current implementation mutates and returns the global list. Prefer working with the passed-in elements parameter and avoid relying on outer scope to make the function reusable and match the specification.

mateacademy-ai-mentor

Review Summary

I am requesting changes because the current implementation does not follow the required function API and this will block the tests from passing. The task requires sortList(listElement) and getEmployees(listElement) to each accept the UL element, work with its <li> children, and in the case of sortList, append the sorted <li>s back into that same list element. Right now, you instead create a top-level array of li elements and pass that array into both functions, and sortList additionally appends to a globally queried ul, so the behavior and signatures don’t match the specification [CHECKLIST ITEMS #1–6].

Critical issues to fix:

At the top of src/scripts/main.js, select the ul element (for example, const ul = document.querySelector('ul');) instead of building an array of lis, and pass this ul into both sortList(ul) and getEmployees(ul).
Change sortList to accept a list element parameter (e.g. listElement), obtain its children via something like const items = Array.from(listElement.querySelectorAll('li'));, sort them by parseToNumber(li.dataset.salary) in descending order, and append the sorted <li> nodes back into listElement inside the function (do not query document.querySelector('ul') inside it anymore).
Change getEmployees to accept the ul element, iterate over its <li> children, and return an array of objects { name, position, salary, age }, making sure salary and age are numbers created by converting from dataset.salary and dataset.age (you’re already using parseToNumber correctly for salary; keep that pattern).

Your parsing helper and the employee object structure are solid, so once you align the function signatures and DOM usage with the task requirements, your solution should meet all checklist items. Keep going—you’re very close, and these changes will make your code cleaner and more reusable as well.

✨ 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-04-22T08:08:13Z

+
+const parseToNumber = (string) => +string.replace(/[^0-9.-]+/g, '');
+
+const sortList = (elements) => {


sortList uses the passed elements (good) but then queries a global ul and appends sorted items there. The function should operate on the provided list element (the function is required to take a list element and append sorted <li> items into that same list). Currently the API and behavior don't match the task: checklist requires sortList(listElement) to obtain its <li> children from that element, sort them by numeric salary and append them back into the same listElement inside the function. Update sortList so it accepts a list DOM element (ul), gets its children, sorts them, and appends them into that same element. Refer to checklist items #3, #5, #6 in task description.

mateacademy-ai-mentor · 2026-04-22T08:08:13Z

 'use strict';

-// write code here
+const list = Array.from(document.querySelectorAll('li'));


The top-level list constant is an array of all li elements. The task expects you to find and get the list (probably the ul element) and then pass that ul into sortList and getEmployees. Right now you pass an array of li elements into both functions. getEmployees and sortList per task should accept the list element (the ul) and operate using its children. Change the code to select the ul (e.g. document.querySelector('ul')) and pass that element to both functions. This addresses checklist steps 1–4.

mateacademy-ai-mentor · 2026-04-22T08:08:13Z

+  sortedList.forEach((el) => ul.appendChild(el));
+};
+
+const getEmployees = (elements) => {


getEmployees currently expects elements to be an array and maps over it. If you change getEmployees to accept the ul element, it should iterate over its <li> children (e.g. Array.from(list.querySelectorAll('li'))), build objects with schema { name, position, salary, age } (salary and age must be numbers), and return the array. Ensure you call getEmployees(ulElement) so tests can find it.

mateacademy-ai-mentor · 2026-04-22T08:08:13Z

+const parseToNumber = (string) => +string.replace(/[^0-9.-]+/g, '');
+
+const sortList = (elements) => {
+  const sortedList = [...elements].sort(
+    (a, b) => parseToNumber(b.dataset.salary) - parseToNumber(a.dataset.salary),


When parsing salary from data attributes you correctly use parseToNumber. Ensure you always read salary from dataset.salary (string) and convert to number before sorting and before returning in objects — this is required by Technical Constraints. The current sortList uses parseToNumber(b.dataset.salary) which is fine if you switch to using the list element's children; keep the helper and its usage.

mateacademy-ai-mentor · 2026-04-22T08:08:13Z

+sortList(list);
+getEmployees(list);


Final calls at bottom pass list (an array) into sortList and getEmployees. After you switch to using the ul element, update these calls to pass the ul element. Also ensure sortList both sorts and appends inside itself and that getEmployees returns the array (and you may capture the returned value if needed). Tests expect both functions to be called.

mateacademy-ai-mentor

Review Summary

I am approving your solution because your sortList(listElement) and getEmployees(listElement) implementations correctly follow the required API and behavior. You accept the UL element as an argument, operate only on its <li> children, and sort by salary in descending order using the parsed numeric value, which matches the task requirements. Your getEmployees function returns an array of objects with the expected schema { name, position, salary, age }, and you correctly convert salary and age to numbers via a helper like parseToNumber, which shows good attention to data types. The DOM usage is nicely scoped to the provided list element, keeping the functions clean and reusable.

As a minor improvement, remember to update the demo link in your README.md by replacing the <your_account> placeholder with your actual GitHub username [CHECKLIST ITEM], and run npm run test locally to confirm everything passes in the project environment. Overall, this is solid, clear work that meets the core functional requirements—well done, keep building on these good practices.

✨ 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-04-22T13:08:00Z

+
+const list = document.querySelector('ul');
+
+sortList(list);


The implementation matches the task API: both sortList(listElement) and getEmployees(listElement) accept the UL element and operate on its
children. sortList sorts by salary (descending) using the dataset.salary value converted with parseToNumber, and appends sorted nodes back into the provided list element. getEmployees returns an array of employee objects with schema { name, position, salary, age } and converts salary and age to numbers. This satisfies the core functional requirements and the helper conversion requirement.

sorted list

a55d81a

mateacademy-ai-mentor suggested changes Apr 21, 2026

View reviewed changes

radykhovska added 5 commits April 21, 2026 16:32

fix code

74aff5d

final fix

2a9290b

fix

0355b9e

task solution

ee308a8

readme file

cafee07

radykhovska requested a review from mateacademy-ai-mentor April 22, 2026 08:06

mateacademy-ai-mentor suggested changes Apr 22, 2026

View reviewed changes

solution

884d73c

radykhovska requested a review from mateacademy-ai-mentor April 22, 2026 13:07

mateacademy-ai-mentor approved these changes Apr 22, 2026

View reviewed changes


		const parseToNumber = (string) => +string.replace(/[^0-9.-]+/g, '');

		const sortList = function (elements) {


		const ul = document.querySelector('ul');

		sortedList.forEach((el) => ul.appendChild(el));


		const parseToNumber = (string) => +string.replace(/[^0-9.-]+/g, '');

		const sortList = (elements) => {

		sortList(list);
		getEmployees(list);

Conversation

radykhovska commented Apr 21, 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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants