Skip to content

add task solution#1090

Open
bodyafell wants to merge 3 commits into
mate-academy:masterfrom
bodyafell:develop
Open

add task solution#1090
bodyafell wants to merge 3 commits into
mate-academy:masterfrom
bodyafell:develop

Conversation

@bodyafell
Copy link
Copy Markdown

Copy link
Copy Markdown

@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

Changes are needed because one of the core required features—numeric sorting for the salary column—is functionally incorrect when salaries contain thousands separators that your own code adds during formatting. In src/scripts/main.js, in the table-sorting logic where you call parseFloat on the cell text for salary, the value "1,000" is parsed as 1, so salary sorting will be wrong once any formatted salary with commas appears; you should strip commas or all non-numeric characters before parsing (e.g. text.replace(/[^\d.-]/g, '') before parseFloat). This directly impacts the correctness of the sorting requirement ([CHECKLIST ITEM #1] and [CHECKLIST ITEM #16]) and needs to be fixed before the solution can be considered complete.

In addition, please adjust the form submit handler so that form.reset() is only called after a successful validation and row add, otherwise the form clears even when validation fails and the user cannot conveniently correct their input. It would also be clearer and more robust to convert employee.age to a number (e.g. const age = Number(employee.age)) before doing the < 18 / > 90 checks, rather than relying on implicit string-to-number coercion. Consider whether you really want the extra position length validation (it is stricter than the task requires and could reject otherwise valid data), and update or remove it accordingly. Finally, for better long-term behavior, prefer removing notification elements from the DOM after the timeout (notification.remove()) instead of only setting display: none, to avoid accumulating hidden elements. Overall your implementation is close and covers most requirements well; once you fix the salary sorting bug and address the smaller issues, it will be in good shape.


✨ 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
Comment on lines +31 to +36
const aNum = parseFloat(
a.cells[columnIndex].textContent.replace('$', ''),
);
const bNum = parseFloat(
b.cells[columnIndex].textContent.replace('$', ''),
);
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Parsing salary for sorting removes only the '$' sign but does not remove thousands separators (commas). parseFloat("1,000") returns 1, so salary column sorting will be incorrect. Remove commas (or non-numeric characters) before parsing, e.g. replace(/[^\\d.-]/g, '') or replace(/,/g, '') as well.

Comment thread src/scripts/main.js Outdated
Comment on lines +154 to +155

form.reset();
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

You always call form.reset() after submit, regardless of whether validation passed. That clears user input even when validation failed — move form.reset() inside the branch where the employee is successfully added (after addEmployeeToTable) so fields remain for correction on error.

Comment thread src/scripts/main.js Outdated
Comment on lines +197 to +206
if (employee.age < 18 || employee.age > 90) {
pushNotification(
20,
20,
'Validation Error',
'Age must be between 18 and 90',
'error',
);

return false;
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The age validation compares employee.age directly to numbers while employee.age comes from FormData as a string. JS will coerce in < and > comparisons, but it's clearer and safer to convert the value to a Number first (e.g. const age = Number(employee.age)) before validating and before using elsewhere.

Comment thread src/scripts/main.js Outdated
Comment on lines +185 to +195
if (employee.position.length < 4) {
pushNotification(
20,
20,
'Validation Error',
'Position must be at least 4 characters long',
'error',
);

return false;
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

You added a validation check for position length (< 4) which is stricter than the task description (task did not require validating position length). This is fine if intentional, but note it is an extra rule beyond the spec and may cause different behavior than expected.

Comment thread src/scripts/main.js
Comment on lines +233 to +235
setTimeout(() => {
notification.style.display = 'none';
}, 2000);
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Notifications are hidden by setting notification.style.display = 'none' after timeout but the element remains in the DOM. To avoid DOM buildup and to keep behavior predictable, consider removing the element (notification.remove()) after hiding (or use a CSS transition and remove on transitionend).

Copy link
Copy Markdown

@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 a couple of validation issues that can block correct behavior and potentially cause tests to fail, even though the main functionality (sorting, selection, form creation, salary formatting, notifications) is well implemented.

Critical issues that need fixing:

  • In src/scripts/main.js, in your validation logic (where you check if (employee.age < 18 || employee.age > 90)), employee.age comes from FormData as a string and is compared directly to numbers. You should explicitly convert it first, e.g. const age = Number(employee.age); and then run the < 18 / > 90 checks and build error messages using age, so the validation is robust and behaves as the tests expect.
  • In the same validation block, you are enforcing a minimum length on position (e.g. position.length < 4), but the task only requires validation for name and age. This extra constraint may cause otherwise valid submissions to be rejected (for example, a short-position name used in tests), so you should remove or relax this check to align exactly with the stated requirements.

Minor/optional improvements (not blockers):

  • Your numeric-column detection for sorting currently uses /\d/.test(cell.textContent), which can misclassify columns where text contains digits; consider either marking numeric columns via headers/attributes or trying to parse both values numerically and falling back to string comparison only if parsing fails.
  • The optional feature of editing table cells on double-click (creating an input with class cell-input, allowing only one edited cell at a time, saving on blur/Enter, and restoring the old value if left empty) is not implemented; you can add a dblclick handler on the table body and manage the input’s lifecycle if you want to satisfy this optional requirement.

Once you adjust the age handling and remove the unnecessary position-length validation, your solution should align well with the task requirements, and the rest of your implementation (sorting, selection, notifications, and form behavior) is on the right track.


✨ 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
Comment on lines +197 to +207

if (employee.age < 18 || employee.age > 90) {
pushNotification(
20,
20,
'Validation Error',
'Age must be between 18 and 90',
'error',
);

return false;
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Age is read from FormData as a string. Comparing employee.age < 18 relies on implicit coercion and can be error-prone. Convert the value explicitly (for example const age = Number(employee.age)) before checking < 18 or > 90 and use age in the comparisons and error messages.

Comment thread src/scripts/main.js Outdated
Comment on lines +185 to +196

if (employee.position.length < 4) {
pushNotification(
20,
20,
'Validation Error',
'Position must be at least 4 characters long',
'error',
);

return false;
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This position length validation is stricter than the task requires (the task only mandates name and age validations). Keeping this check may cause otherwise valid submissions to be rejected and tests to fail. Consider removing or relaxing it unless you intentionally want to validate position length.

Comment thread src/scripts/main.js
rows = Array.from(tbody.rows);

rows.sort((a, b) => {
if (/\d/.test(a.cells[columnIndex].textContent)) {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The numeric-column detection uses a simple /\d/.test(...) on a cell's textContent. That can misclassify columns if a textual cell contains digits. Consider a more robust approach: either mark numeric columns via a header/data attribute or attempt to parse both cell values (after stripping non-numeric chars) and fall back to text comparison only when parsing yields NaN.

Comment thread src/scripts/main.js
activeRow = target;
}
});

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The optional double-click cell editing feature (append an input with class cell-input, allow only one edited cell, save on blur/Enter and restore initial value if input is empty) is not implemented. If you need the optional requirement, add a dblclick handler on the table body to create the input element and handle blur/keydown lifecycle.

Copy link
Copy Markdown

@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 move forward while you continue improving the code. You implemented all the core requirements very well: sorting with ASC/DESC toggling, single-row selection via the active class, script-generated new-employee-form with correct fields and data-qa attributes, validation for name and numeric age, formatted salary, and clear success/error notifications that match the task expectations. One area to refine is numeric-column detection in src/scripts/main.js: instead of using /\d/.test(cell.textContent) to decide whether to sort numerically, try parsing both cell values (e.g., strip non-numeric characters and use parseFloat) and only do numeric comparison when both parsed values are valid numbers; otherwise, fall back to localeCompare for safer behavior [CHECKLIST ITEM #1]. Another optional improvement is to implement the double-click cell-editing behavior described in the task (single .cell-input at a time, save on blur/Enter, restore original value if input is empty) by adding a dblclick handler on the table body near your row-selection logic [CHECKLIST ITEM #31]. As a minor polish, you might also normalize the age cell to a Number before inserting it into the table so it is consistent with how you validate it. Overall, your code is clean, readable, and meets the main requirements, and these suggestions are good next steps to deepen your understanding and make the solution more robust.


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

Footnotes

  1. Rate AI review example

Comment thread src/scripts/main.js
rows = Array.from(tbody.rows);

rows.sort((a, b) => {
if (/\d/.test(a.cells[columnIndex].textContent)) {
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 /\d/ to detect numeric columns (checking a single cell's text for any digit) can misclassify some columns that contain digits inside text. Consider trying to parse both cell values to numbers (e.g. strip non-numeric chars and use parseFloat) and check whether both parsed values are valid numbers (!Number.isNaN(parsed)) before doing numeric comparisons; otherwise fall back to localeCompare. This will make numeric detection more robust for edge cases.

Comment thread src/scripts/main.js
});

rows.forEach((row) => tbody.appendChild(row));
});
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The optional cell-editing feature (double-click to create an input with class cell-input, allow only one editor at a time, save on blur/Enter, restore original value if input empty) is not implemented. If you decide to add it, attach a dblclick handler on the table body (e.g. near the row-selection logic) and follow the rules in checklist item #31 (only one editor, save on blur/Enter, restore previous value if empty).

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