task solution#1112
Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
The implementation has critical blockers that prevent task completion. Table sorting is completely missing - checklist items #1 and #2 require sorting by column headers in ASC/DESC order, but no sorting code exists. Row selection is broken - the click handler only removes the 'active' class from existing rows but never adds it to the clicked row (items #3 and #4). Required field validation is missing - the form should validate that all fields are non-empty before processing (item #18). Additionally, the success notification appears on line 69 BEFORE the employee is added to the table on line 81, which creates incorrect UX. Eight of fourteen Cypress tests are failing due to these issues. These are not minor problems - the core table functionality (sorting and selection) is fundamentally broken and must be fixed before the PR can be approved.
✨ 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
| tbody.addEventListener('click', (e) => { | ||
| const row = e.target.closest('tr'); | ||
|
|
||
| if (!row) { | ||
| return; | ||
| } | ||
|
|
||
| const active = tbody.querySelector('.active'); | ||
|
|
||
| if (active) { | ||
| active.classList.remove('active'); | ||
| } |
There was a problem hiding this comment.
|
|
||
| const employeeName = form.elements.name.value; | ||
| const position = form.elements.position.value; | ||
| const age = +form.elements.age.value; | ||
| const salary = +form.elements.salary.value; | ||
| const office = form.elements.office.value; |
There was a problem hiding this comment.
The form is missing required field validation. Checklist item #18 states all fields are required. Empty inputs should trigger an error notification before processing other validations.
|
|
||
| showNotification('success', 'Success', 'Employee added successfully'); |
There was a problem hiding this comment.
The success notification is shown on line 69 BEFORE adding the employee to the table (line 81). This creates incorrect UX - the notification should appear after the employee is successfully added. Move the success notification call to after tbody.append(tr).
| 'use strict'; | ||
|
|
||
| // write code here | ||
| const tbody = document.querySelector('tbody'); | ||
|
|
||
| tbody.addEventListener('click', (e) => { | ||
| const row = e.target.closest('tr'); | ||
|
|
||
| if (!row) { | ||
| return; | ||
| } | ||
|
|
||
| const active = tbody.querySelector('.active'); | ||
|
|
||
| if (active) { | ||
| active.classList.remove('active'); | ||
| } | ||
| }); |
| form.addEventListener('submit', (e) => { | ||
| e.preventDefault(); | ||
|
|
||
| const employeeName = form.elements.name.value; | ||
| const position = form.elements.position.value; | ||
| const age = +form.elements.age.value; | ||
| const salary = +form.elements.salary.value; | ||
| const office = form.elements.office.value; | ||
|
|
||
| if (employeeName.length < 4) { | ||
| showNotification( | ||
| 'error', | ||
| 'Error', | ||
| 'Name must be at least 4 characters long', | ||
| ); | ||
|
|
||
| return; | ||
| } | ||
|
|
||
| if (age < 18 || age > 90) { | ||
| showNotification('error', 'Invalid Age', 'Age must be between 18 and 90'); | ||
|
|
||
| return; |
There was a problem hiding this comment.
The form validation logic runs but doesn't check if required fields are empty. According to checklist item #18, all fields must be required. Add a check at the beginning of the submit handler to validate that no field is empty.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
The code has a critical bug that violates a core requirement. On line 10, currentDirection is initialized to 'desc', which causes the first column click to sort in DESC order instead of the required ASC order. The task explicitly states that "When users click on one of the table headers, the table should be sorted in ASC order." To fix this, initialize currentDirection with 'asc' instead so the first click sorts ascending and subsequent clicks properly toggle to descending. This is a blocking issue because the sorting direction is incorrect for the primary interaction (first click), which directly breaks a specified requirement.
✨ 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
|
|
||
| const currentDirection = sordDirections[columnIndex] || 'desc'; |
There was a problem hiding this comment.
Bug: First click on a column sorts in DESC instead of ASC. The requirement states 'When users click on one of the table headers, the table should be sorted in ASC order'. Currently, currentDirection defaults to 'desc', so the first click toggles to 'asc', but the SORT happens with newDirection (which is 'asc'), making the result DESC. To fix: initialize currentDirection with 'asc' (or a value that toggles to 'asc' on first click), so the sort uses 'asc'.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
The sort state isn't being reflected in the DOM. Add 'sort-asc' and 'sort-desc' classes to the active column header during sorting, and remove them from other headers. This is required for the Cypress tests to pass.
✨ 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
3 participants
No description provided.