solution 1.0

[Viktor-1995]

• https://viktor-1995.github.io/js_employees_table_DOM/

[mateacademy-ai-mentor]

Review Summary

I am requesting changes because there are still a few critical functional issues that violate the task requirements and are causing tests to fail.

The most important problems are: (1) header sorting does not toggle correctly between ASC and DESC on repeated clicks in src/scripts/main.js (in the header click handler), so you should store and flip a sort direction flag per column instead of relying only on the previous header text; and (2) the office <select> (created in src/scripts/main.js and rendered in index.html) is not marked required, and the submit handler returns early when any field is missing without calling pushNotification, so you need to set select.required = true and, on invalid/missing data, show an error notification with data-qa="notification" instead of silently exiting. Cypress is also failing on the case “warning notification on position field with invalid input”, which means the position field does not currently trigger a notification when invalid; add validation for the position input (similar to name/age) and call pushNotification('error', '...') or a properly styled warning type when the value is empty or invalid. In pushNotification you currently map the custom warning type to the error class; either pass 'error' for invalid inputs or update the mapping so 'warning' uses the .notification.warning styling, keeping type and CSS class consistent. For row selection, the click handler relies on e.target.parentElement and overwrites row.className, which is brittle; instead, use e.target.closest('tr') with a null check and classList.remove('active') / classList.add('active') so it works even if the user clicks nested elements.

Once you fix the sort toggling, make all form fields (including office) truly required with notifications for invalid submissions, and ensure position validation produces a visible notification, your implementation should fully satisfy the specification and pass the end-to-end tests. You already have most of the functionality in place (dynamic form, notifications, salary formatting, row selection), so focus on tightening these edge cases and your solution will be in great 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]

The sorting state is only tracked by the previous header text (prev) so after the first click the code always treats subsequent clicks on the same header as the "second click" (descending) — it doesn't toggle ASC/DESC on each click. Track and toggle sort direction (e.g. a map of header -> direction or a boolean that toggles for the same header) rather than relying only on prev.

[mateacademy-ai-mentor]

The for office is missing the required attribute. The task requires all fields to be required, so set selectLabel.required = true (or add the required attribute) when creating the select.

[mateacademy-ai-mentor]

This early return silently exits when some fields are missing. The spec requires showing a notification if form data is invalid — you should push an error notification here instead of returning without feedback.

[mateacademy-ai-mentor]

Using e.target.parentElement assumes the clicked element is always a cell (td). It's brittle (clicking on nested elements or whitespace could break). Use e.target.closest('tr') to reliably find the row, and guard against null.

[mateacademy-ai-mentor]

Sorting state is tracked only by the last header text (prev) and never toggled back to ASC on subsequent clicks. As written: first click on a header sorts ASC, second click sorts DESC, but further clicks remain DESC because prev never changes to track direction. To meet the requirement you should track sort direction (e.g. a boolean per header or a single variable that toggles each time the same header is clicked) and flip it on each click so the header toggles ASC <-> DESC.

[mateacademy-ai-mentor]

The office is created but not marked as required. The spec says "All fields are required" and there must be a qa attribute for office (which exists). Add selectLabel.required = true; so the select cannot be left empty and the required constraint holds.

[mateacademy-ai-mentor]

This early-return silently exits when any field is missing: if (!firstName || !position || !office || !age || !salary) { return; }. The task requires showing a notification when form data is invalid. Replace this silent return with a call to pushNotification(...) using an error type (and include data-qa="notification"). Also consider a more precise check (so 0 values are handled correctly) and leverage the required attributes on inputs/select.

[mateacademy-ai-mentor]

pushNotification maps a 'warning' type to the 'error' class (else if (type === 'warning') { div.className = div.className + ' error'; }). The task requires error/success classes on notifications. Either pass 'error' for invalid inputs (preferred) or map 'warning' to the 'warning' class so the CSS .notification.warning is used. Using 'warning' but styling as 'error' is confusing and prevents use of the provided warning styling if intended.

[mateacademy-ai-mentor]

Review Summary

I am requesting changes because there are a few functional issues that conflict with the task requirements and are likely to cause test failures. In src/scripts/main.js, the row selection handler (in the tbody click listener near the sorting/selection logic) relies on e.target.parentElement and assigns row.className = '', which is brittle and can break when clicking nested elements or when rows have multiple classes; you should instead use const row = e.target.closest('tr'); (with a null check) and classList.remove('active') / classList.add('active') so exactly one row has the active class at a time [CHECKLIST ITEM #48, #49]. The notification system is also inconsistent: your validation branches currently call pushNotification with type 'warning', but in pushNotification (in src/scripts/main.js) the 'warning' case maps to the 'error' CSS class instead of adding ' warning', so tests expecting .notification.warning or clear error notifications for invalid data will fail; either change the mapping to add the warning class or change callers to pass 'error' for invalid submissions [CHECKLIST ITEM #42, #45]. The missing-data validation around the form submit handler uses a generic falsy check like if (!firstName || !position || !office || !age || !salary), which treats 0 as missing and is not aligned with robust validation; please switch to form.checkValidity() or explicit checks such as firstName.trim() === '' and isNaN(age) so all required fields are handled correctly and still trigger a visible error notification when invalid [CHECKLIST ITEM #8, #46]. Additionally, the optional cell-editing feature (double-click to edit a cell, single editable cell at a time, save on blur/Enter and revert empty edits) is not implemented, and if your tests expect it you will need to add a dblclick handler on table cells that inserts an input.cell-input, ensures only one editor exists, and saves or reverts appropriately [CHECKLIST ITEM #15–#20]. As minor improvements, consider cleaning up notifications by removing them from the DOM instead of just hiding them and ensuring your submit button explicitly uses type="submit" for semantic correctness, though these are not blockers. Overall, you’ve implemented most of the core features (sorting, dynamic form, notifications, salary formatting) well; once you fix the row selection logic, notification type handling, and validation robustness, your solution should align fully with the checklist and pass the tests.

---

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

Row selection handler is brittle. Instead of iterating rows and testing row.className.includes('active') then setting row.className = '' (which can wipe other classes), remove the active class with classList.remove('active') and add it with classList.add('active') on the clicked row. Also don't rely on e.target.parentElement; use e.target.closest('tr') and check for null before adding the class so clicks on nested elements still select the correct row.

[mateacademy-ai-mentor]

The missing-data validation uses a falsy-check: if (!firstName || !position || !office || !age || !salary). This treats numeric 0 as missing and is brittle. Use form.checkValidity() or explicit checks (e.g. firstName.trim() === '', position.trim() === '', isNaN(age)) to validate fields reliably. Also, when showing the notification for missing data, prefer using the 'error' type (see mapping issues below) so the notification class and styling match expectations.

[mateacademy-ai-mentor]

Validation branches (name length, position length, age range) call pushNotification with type 'warning'. The task expects visible error notifications for invalid form data. Either change these calls to use 'error' (so the notification gets the .notification.error class) or ensure pushNotification maps 'warning' to add the warning class. At the moment, using 'warning' here combined with the mapping below makes the behavior inconsistent and may hide or misclassify notifications.

[mateacademy-ai-mentor]

Inside pushNotification, the warning type is currently mapped to the 'error' class (div.className = div.className + ' error'). This is inconsistent: CSS contains a .notification.warning style. Change this branch to add ' warning' instead of ' error', or update callers to pass 'error' for invalid submissions. Keep the notification type and resulting CSS class consistent.

[mateacademy-ai-mentor]

The cell editing feature (double-click cell to edit, single editable cell, save on blur/Enter, revert on empty submit) is not implemented. The task marks it optional in the description, but if tests expect it you should implement dblclick handlers that insert an input with class cell-input, ensure only one input exists at a time, and handle blur/Enter saving while reverting empty submissions to the original value.

[mateacademy-ai-mentor]

Row selection uses e.target.parentElement and sets row.className directly. This is brittle (clicks on nested elements or text nodes may not behave as expected) and overwrites other classes. Use e.target.closest('tr') with a null check to find the clicked row, and use classList.remove('active') / classList.add('active') to toggle the class so only one row is active at a time (matches checklist items #3, #4, #5 and previous feedback items #48/#49).

[mateacademy-ai-mentor]

Review Summary

I am approving your solution as an educational override so you can keep progressing while you refine a few important details. You’ve done a solid job implementing the core behaviors: sortable table headers, dynamic form creation with the right fields and data-qa attributes, salary formatting, and a notification system that uses the expected data-qa="notification" and status classes. The main things to improve are: (1) row selection logic in src/scripts/main.js around the click handler (near the existing e.target.parentElement) – use const row = e.target.closest('tr') and classList so exactly one row has the active class [CHECKLIST ITEM #X], and (2) form validation (near the if (!firstName || !position || !office || !age || !salary) check) – falsy checks treat 0 as missing, so prefer form.checkValidity() or explicit checks like firstName.trim() === '' and Number.isNaN(age) for numeric fields. It would also be good to adjust your submit handling so the button has type="submit" and the logic runs on the form’s submit event, which improves accessibility and lets native validation work. Finally, if your task or tests expect it, consider implementing the optional double‑click cell editing: on dblclick of a td, insert a single input.cell-input, save on blur/Enter, and revert when left empty. Overall, your structure and DOM work are strong—treat these changes as polishing steps that will make your solution more robust and closer to production quality.

---

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

Footnotes

1. Rate AI review example ↩

[mateacademy-ai-mentor]

Selection logic is brittle: using e.target.parentElement fails when clicking nested elements and can select wrong element. Use const row = e.target.closest('tr') with null check. Also current loop clears classes by checking string includes — prefer row.classList.remove('active') and ensure exactly one row has active class at a time (remove from previously active row only). This violates checklist items about robust row selection handling.

[mateacademy-ai-mentor]

Validation treats numeric fields incorrectly because !age and !salary will treat 0 as missing and may mis-handle valid numeric inputs. Replace with form.checkValidity() or explicit checks like firstName.trim() === '', position.trim() === '', office === null and Number.isNaN(age) / Number.isNaN(salary). This addresses required-field robustness per checklist.

[mateacademy-ai-mentor]

Name/position length validations are applied, but earlier generic missing-data check may intercept and show less specific message; ensure missing-field validation allows the specific length and range checks to run, and that errors use consistent notification type ('error') as required. Also ensure name length <4 triggers error notification (it does) but see previous comment about missing-data ordering.

[mateacademy-ai-mentor]

Notification creation uses div.className = 'notification' then appends classes by string concatenation. While functional, it's safer to use div.classList.add('success'|'error'|'warning'). Tests expect data-qa="notification" and classes error/success — current mapping supports that, but callers previously used inconsistent types in earlier reviews; keep using 'error' for invalid submissions (you do).

[mateacademy-ai-mentor]

Form submit button is created without type="submit". Currently you prevent default on button click handler, but semantically and for accessibility it should be <button type="submit">. Alternatively, attach submit listener on form. Tests may rely on form semantics.

[mateacademy-ai-mentor]

Cell-editing (double-click to edit) feature is missing. The task marks this as optional but tests may expect it. Implement dblclick on tbody cells to append an input.cell-input, remove cell text, ensure only one editor exists, save on blur or Enter, and revert to initial value if input empty. See checklist items for exact behavior.

[mateacademy-ai-mentor]

Selection handler is brittle: using e.target.parentElement can fail when clicking nested elements (e.g., if a cell contains markup). Use const row = e.target.closest('tr') and guard for null. Also current code clears classes by checking row.className.includes('active') then row.classList.remove('active') — better to always remove active from all rows via tbody.querySelectorAll('tr').forEach(r => r.classList.remove('active')) then add to the clicked row. This relates to checklist items: only one active row and robust selection. (See lines 56–63)

[mateacademy-ai-mentor]

Validation treats numeric values incorrectly: the required check if (!firstName || !position || !office || !age || !salary) treats 0 as missing and will mis-handle valid numeric inputs. Per requirements use form.checkValidity() or explicit checks (firstName.trim() === '', position.trim() === '', !office, isNaN(age), isNaN(salary)) so all fields are validated correctly and missing data triggers an error notification. (See lines 126–136)

[mateacademy-ai-mentor]

Submit button created without explicit type="submit". Currently you preventDefault on click — better to set button.type = 'submit' and handle the form's submit event instead of button click to preserve semantics and allow form.checkValidity() to work properly. (See lines 73–76 and 126–127)