unfinished task

[Kit3AWP]

- [DEMO LINK](https://Kit3AWP.github.io/js_employees_table_DOM/)

!! Unfinished task

[mateacademy-ai-mentor]

Review Summary

I am requesting changes because several core requirements are not yet implemented or are implemented incorrectly, which currently blocks the main user flows and causes the Cypress tests to fail.

Critical issues that must be fixed:

• In src/scripts/main.js, the row selection logic clears the wrong element: inside the rows.forEach(...) you always call row.classList.remove('active') (the clicked row) instead of the iterated row, so multiple rows can remain active; update it to something like rows.forEach(r => r.classList.remove('active')) before adding active to the clicked row.
• The form is missing the required office <select> with data-qa="office" and exactly six options (Tokyo, Singapore, London, New York, Edinburgh, San Francisco); add this select (inside a label, before the Age input) so the tests can find and use it.
• The handleSubmit function is effectively empty: it must validate that all fields (including office) are filled, ensure name has at least 4 characters, age is between 18 and 90 inclusive, convert and format the salary into the same currency format used in the table (e.g. $4,500), append a new <tr> into the table <tbody> on success, and create a notification element with data-qa="notification" and class success or error depending on the outcome.
• Right now there is no notification rendering or field-level validation feedback, but the tests expect warnings when name/position are invalid and when age is out of bounds, so you should show an error notification with an appropriate message whenever validation fails.

As a minor improvement, you can also mark the inputs as required in HTML and then still enforce the same rules in JS for more precise control. You already have a solid base with sorting and most of the form structure; once you implement the submit logic, validation, notifications, office select, and fix the row-selection bug, your solution should be in good shape.

---

✨ 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.

Footnotes

1. Rate AI review example ↩

2. Re-request AI review example ↩

3. Add AI review comments example ↩

[mateacademy-ai-mentor]

You collect all tbody rows into rows but the forEach callback doesn't use the iterated item. The code currently calls row.classList.remove('active') for the clicked row repeatedly—this won't remove active from other rows. Change the callback to receive each row (for example rows.forEach(r => r.classList.remove('active'))) so only one row remains active.

[mateacademy-ai-mentor]

The form must include an office select with exactly 6 options (Tokyo, Singapore, London, New York, Edinburgh, San Francisco) and the select must have data-qa="office". I don't see any creation of that select — add it (inside a label like the other inputs) before the Age input so tests can find data-qa="office".

[mateacademy-ai-mentor]

The handleSubmit function is empty. Per requirements you must: validate all fields are present, ensure name length is at least 4 chars, ensure age is between 18 and 90 (inclusive), convert/format the salary string like in the table, add a new row to the table when valid, and show a notification with data-qa="notification" and class success or error depending on the result. Implement these behaviors here and append the new tr into the tbody.

[mateacademy-ai-mentor]

When adding the salary to the table, convert and format the numeric value into the expected table format (e.g. include currency sign and thousands separators) instead of directly inserting raw input string or number — tests expect the table-style salary formatting.

[mateacademy-ai-mentor]

Review Summary

I am requesting changes because there are still a few functional gaps that conflict with the task requirements and may cause tests to fail. First, in src/scripts/main.js, the cell-editing logic allows multiple cells to be edited at once; the requirement is that only one cell is editable at a time, so you should check for an existing .cell-input globally (e.g., via document.querySelector('.cell-input')) and either prevent starting a new edit or save/close the previous one before creating a new input. Second, when building the notification element in src/scripts/main.js, you append the titleElement twice, which produces an incorrect DOM structure; remove the initial single append and only append titleElement together with messageElement once so the notification children are in the expected order. Third, all form fields are logically required but the HTML required attribute is not set on the inputs and the select; the task expects these to be marked required in markup as well, so set required = true (or the equivalent attribute) on each field when you create them. These three issues directly touch explicit checklist items and automated checks, so they need to be corrected before the solution can be considered complete. Aside from that, your overall implementation of sorting, row selection, validation, notifications, and inline editing behavior is solid and close to the desired behavior. Once you address these specific points, your solution should fully align with the project 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.

Footnotes

1. Rate AI review example ↩

2. Re-request AI review example ↩

3. Add AI review comments example ↩

[mateacademy-ai-mentor]

The code checks only the clicked cell for an existing input (cell.querySelector('input')) so it prevents editing the same cell twice but does not prevent another cell from already being edited. The requirement says only one cell can be edited at a time. Consider checking globally for an existing editor (e.g. if (document.querySelector('.cell-input')) return;) or programmatically saving/closing any existing cell editor before creating a new one.

[mateacademy-ai-mentor]

You append the titleElement twice: first with notification.append(titleElement); and then again with notification.append(titleElement, messageElement);. That moves/duplicates the node and results in incorrect DOM structure/order. Remove the first append and only append both elements once (or append titleElement and then messageElement).

[mateacademy-ai-mentor]

Form inputs/select are not marked with the HTML required attribute. The task states all fields are required — while you correctly validate in JS, tests or accessibility expectations may check for the required attribute. Add required to each input and the select (e.g. input.required = true / inputSelect.required = true) so the fields are explicitly required in markup as well as in JS.

[mateacademy-ai-mentor]

Review Summary

Your solution is approved because it correctly implements the key behaviors: sortable table columns with ASC/DESC toggling, single-row selection via the active class, dynamic employee form creation with required fields and data-qa attributes, validation with clear error notifications, and inline cell editing that allows only one editable cell at a time. Since there are only minor improvements left, consider this an educational approval to help you keep progressing while you refine your code quality. You did especially well ensuring validation rules for name length and age bounds, formatting salary values, and wiring notifications with the correct data-qa="notification" plus error/success classes. As a small refinement, where you call form.setAttribute('novalidate', true), using form.noValidate = true would be a bit clearer and more idiomatic in modern JavaScript. Another minor improvement would be to make your numeric sort logic more robust so that future columns with decimal or negative numbers (e.g., -5.5) are handled correctly instead of stripping all non-digits. Overall, your implementation is solid and fulfills the checklist requirements, and these small tweaks will help you write even more maintainable and extensible code going forward.

---

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

Footnotes

1. Rate AI review example ↩

[Anton-Kuchmasov]

Well done!