Skip to content

Develop#1923

Open
uselessmorning wants to merge 4 commits intomate-academy:masterfrom
uselessmorning:develop
Open

Develop#1923
uselessmorning wants to merge 4 commits intomate-academy:masterfrom
uselessmorning:develop

Conversation

@uselessmorning
Copy link

Copy link

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

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 critical gaps in meeting the core task requirements in src/scripts/main.js. Your getEmployees function currently returns objects with only name and salary, and name is taken from item.textContent, which mixes multiple fields; the task explicitly requires the schema { name, position, salary, age } with name and position taken from specific child elements and age parsed to a number. Please update getEmployees so that for each list item it selects the correct elements (e.g. .name, .position) and builds objects like:

{
  name: item.querySelector('.name').textContent.trim(),
  position: item.querySelector('.position').textContent.trim(),
  salary: toNum(item.dataset.salary),
  age: Number(item.dataset.age)
}

Additionally, your toNum helper assumes the input is always a string and calls replace directly; if the data-salary attribute is missing or undefined, this will throw at runtime and break both sorting and employee extraction. To fix this, guard the input or coerce it to a string first, for example by doing something like const toNum = (str) => Number(String(str || '').replace(/[$,]/g, '')); so it safely returns NaN/0 instead of crashing. Once these issues are addressed, your sorting logic and overall structure should align well with the requirements, and you’ll have a robust implementation that passes the checklist.


✨ 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


// write code here

const toNum = (str) => Number(str.replace(/[$,]/g, ''));

Choose a reason for hiding this comment

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

toNum assumes str is a string and calls replace directly. If str is undefined (missing data attribute) this will throw. Consider guarding the input or converting to string first, e.g. const toNum = (str) => Number(String(str || '').replace(/[$,]/g, '')); so it safely returns NaN/0 instead of throwing. This relates to the helper requirement to convert salary strings to numbers.

Comment on lines 26 to 31
return items.map((item) => {
return {
name: item.textContent.trim(),
salary: toNum(item.dataset.salary),
};
});

Choose a reason for hiding this comment

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

getEmployees returns objects with only name and salary, but the task requires the schema { name, position, salary, age }. Also name is taken from item.textContent.trim(), which will include other fields and whitespace. Update this function to select specific child elements (for example item.querySelector('.name') and item.querySelector('.position')) and parse age to a number. Example shape to return:

{
  name: item.querySelector('.name').textContent.trim(),
  position: item.querySelector('.position').textContent.trim(),
  salary: toNum(item.dataset.salary),
  age: Number(item.dataset.age)
}

This change is required to meet the checklist item demanding the exact employee schema.

Copy link

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

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 one core requirement of the task is not yet met, which will cause incorrect output for getEmployees. In src/scripts/main.js, getEmployees currently builds each employee object using item.textContent for name and item.dataset.position for position, so the returned objects do not match the required { name, position, salary, age } schema.

The critical fixes you need to make are:

  • For name, query the specific child element (e.g. item.querySelector('.name')), guard for a missing node, and use .textContent.trim(); do not use item.textContent because it mixes position, salary, etc. into the name.
  • For position, query the corresponding child element (e.g. item.querySelector('.position')) and use its .textContent.trim() instead of reading from item.dataset.position.
  • Ensure that age is stored as a Number in the returned objects (not a string), consistent with the task description.

Everything else (like your toNum helper and sortList behavior) is on the right track, so once you update getEmployees to pull name and position from their child elements and construct properly typed objects, the implementation should satisfy the task requirements. Keep going—you’re very close to a fully compliant solution.


✨ 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


return items.map((item) => {
return {
name: item.textContent.trim(),

Choose a reason for hiding this comment

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

name is taken from item.textContent, which will include other text (position, etc.). The requirement expects name to be read from the specific child element (e.g. .name) and trimmed. Consider using item.querySelector('.name').textContent.trim() and fallback to an empty string if the node is missing.

return items.map((item) => {
return {
name: item.textContent.trim(),
position: item.dataset.position,

Choose a reason for hiding this comment

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

position is read from item.dataset.position. The task requires position to come from the specific child element (e.g. .position). Use item.querySelector('.position').textContent.trim() so the returned object matches the required schema.

Copy link

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

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 a few remaining details. You did a solid job implementing toNum, sortList, and getEmployees, correctly converting salary and age to numbers and sorting the list by salary in descending order as required. There are two important robustness issues left in src/scripts/main.js: when you do item.querySelector('.name').textContent.trim() and item.querySelector('.position').textContent.trim(), you should first store the node in a variable and check it exists (e.g. const nameEl = item.querySelector('.name'); const name = nameEl ? nameEl.textContent.trim() : '') so you don’t risk a runtime error if the element is missing. Also, when you call getEmployees(employeesList);, make sure to capture the returned value in a variable (for example const employees = getEmployees(employeesList);) so you truly “get an array of employees” as the task describes. A smaller improvement is to consider a safe fallback for age if data-age is missing or invalid, so it doesn’t silently become NaN. Overall, your structure and logic show good understanding of DOM traversal and sorting; addressing these guards and the captured return value will make your solution more robust and production-ready.


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

Footnotes

  1. Rate AI review example


return items.map((item) => {
return {
name: item.querySelector('.name').textContent.trim(),

Choose a reason for hiding this comment

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

Accessing item.querySelector('.name').textContent.trim() without checking the query result can throw if the .name element is missing. Per the requirements you must guard for a missing node before using .textContent (for example: const nameEl = item.querySelector('.name'); name: nameEl ? nameEl.textContent.trim() : '').

return items.map((item) => {
return {
name: item.querySelector('.name').textContent.trim(),
position: item.querySelector('.position').textContent.trim(),

Choose a reason for hiding this comment

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

Same for .position: calling item.querySelector('.position').textContent.trim() assumes the node exists. Add a null check or optional chaining with a fallback to avoid runtime errors (e.g. const posEl = item.querySelector('.position'); position: posEl ? posEl.textContent.trim() : '').

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

sortList(employeesList);
getEmployees(employeesList);

Choose a reason for hiding this comment

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

You call getEmployees(employeesList); but do not store its return value. The task asks to "get an array of employees" — capture the result so it's available (for example: const employees = getEmployees(employeesList);).

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