Skip to content

add solution#2009

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

add solution#2009
m-berezovskyy wants to merge 2 commits into
mate-academy:masterfrom
m-berezovskyy:develop

Conversation

@m-berezovskyy

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 there are still a couple of critical requirement mismatches in getEmployees that will cause the tests to fail. First, on src/scripts/main.js where you build the employee object (inside getEmployees), you currently return salary: item.dataset.salary; this keeps salary as a string, but the task requires a numeric salary converted via your helper, e.g. salary: parseSalary(item.dataset.salary) so that the returned objects match the expected schema [CHECKLIST ITEM #2, #5, #7]. Second, your name field is currently taken from item.textContent.trim(), which will include all text inside the <li> (such as position and possibly salary), not just the employee name; the requirements expect name to come from the dedicated name source (typically item.dataset.name or a specific child element like .employee-name) so the object’s name is accurate [CHECKLIST ITEM #2, #5]. Please update getEmployees to read the name from the appropriate dataset attribute or DOM child, and to use parseSalary when setting salary in the returned object array. Once these two issues are fixed, the rest of your implementation (sorting logic, helper usage, DOM selection) already aligns well with the task, and your solution should pass the tests. Keep going—you’re very close, and these adjustments will make your solution fully compliant with the specification.


✨ 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
return {
name: item.textContent.trim(),
position: item.dataset.position,
salary: item.dataset.salary,

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 return salary: item.dataset.salary, which is still a string. According to the description and checklist items #5 and #7, salary must be converted from the data attribute string to a number using your helper function (e.g., parseSalary(item.dataset.salary)).

Comment thread src/scripts/main.js Outdated

return items.map((item) => {
return {
name: 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.

Using item.textContent.trim() for name will include all text inside the list item, not just the employee name. The task expects an employee object with correct name field, so consider reading the name either from a specific dataset field (e.g., item.dataset.name) or the appropriate child element in the DOM, depending on the HTML structure.

@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 because it meets all of the core functional requirements of the task. Your parseSalary helper correctly converts salary strings from data-salary into numbers and is reused consistently in both sortList and getEmployees, which matches the requirements. The sortList(listElement) function reads salaries from data attributes, sorts the list items in descending order, and re-appends them to the <ul>, so the visual list ordering will be correct. The getEmployees(listElement) function returns an array of employee objects with the required shape (name, position, salary, age), and importantly, salary is numeric, not a string. The strategy you use to extract name from the <li> text nodes is reasonable given the available HTML context and does not conflict with any explicit requirement.

If the final HTML template exposes a dedicated element like .employee-name or a data-name attribute, you could simplify and harden the name extraction by reading from that instead of manual text-node filtering. Overall, your code is clean, focused, and should satisfy the tests and task expectations—well done.


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

Footnotes

  1. Rate AI review example

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