version 1#1095
Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am requesting changes because there is at least one clear checklist violation that will likely cause tests to fail and block task completion. The Position input you create in src/scripts/main.js (around the form markup, noted near line 68) is missing the required attribute, even though your validation logic later checks positionValue (around line 95) and the task explicitly says all fields must be required [CHECKLIST ITEM #25]; you should add required to that input so the markup and validation are consistent.
Critical issues to fix:
- Add the
requiredattribute to the Position input in the form you generate so all fields are marked required as specified.
Important but non-blocking improvements (fix if you have time):
- Your labels/button text and notification language are mixed (English for labels/button vs Ukrainian for notifications); align these to the language expected by the task screenshot so visible text is consistent [CHECKLIST ITEM #20].
- When setting notification classes, you currently assign
element.className = ..., which overwrites any existing classes; switch toelement.classList.add('error')/classList.add('success')so you preserve the base notification class and only add modifiers [CHECKLIST ITEM #34]. - Consider trimming cell text (
cellText.trim()) before sorting so leading/trailing spaces don’t affect the sort order.
Overall, your sorting, row selection, form creation, validation, and notification logic are well-structured and close to fully compliant; once you address the required Position field and, ideally, the smaller points above, your solution should be ready 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
|
|
||
| form.innerHTML = ` | ||
| <label>Name: <input name="name" type="text" data-qa="name" required></label> | ||
| <label>Position: <input name="position" type="text" data-qa="position"></label> |
There was a problem hiding this comment.
The form's Position input is missing the required attribute. Checklist item #25 requires all fields to be required; add required to the position input in the form markup.
| form.innerHTML = ` | ||
| <label>Name: <input name="name" type="text" data-qa="name" required></label> | ||
| <label>Position: <input name="position" type="text" data-qa="position"></label> | ||
| <label>Office: | ||
| <select name="office" data-qa="office" required> | ||
| <option value="Tokyo">Tokyo</option> | ||
| <option value="Singapore">Singapore</option> | ||
| <option value="London">London</option> | ||
| <option value="New York">New York</option> | ||
| <option value="Edinburgh">Edinburgh</option> | ||
| <option value="San Francisco">San Francisco</option> | ||
| </select> | ||
| </label> | ||
| <label>Age: <input name="age" type="number" data-qa="age" required></label> | ||
| <label>Salary: <input name="salary" type="number" data-qa="salary" required></label> | ||
| <button type="submit">Save to table</button> |
There was a problem hiding this comment.
The label/button texts should match the screenshot/localization per checklist item #20. You use Ukrainian messages for notifications, but form labels and button are in English; ensure this matches the provided screenshot. If the screenshot expects specific texts, update labels/button accordingly.
| <label>Position: <input name="position" type="text" data-qa="position"></label> | ||
| <label>Office: | ||
| <select name="office" data-qa="office" required> | ||
| <option value="Tokyo">Tokyo</option> | ||
| <option value="Singapore">Singapore</option> | ||
| <option value="London">London</option> | ||
| <option value="New York">New York</option> | ||
| <option value="Edinburgh">Edinburgh</option> | ||
| <option value="San Francisco">San Francisco</option> | ||
| </select> | ||
| </label> | ||
| <label>Age: <input name="age" type="number" data-qa="age" required></label> | ||
| <label>Salary: <input name="salary" type="number" data-qa="salary" required></label> | ||
| <button type="submit">Save to table</button> | ||
| `; | ||
|
|
||
| document.body.appendChild(form); | ||
|
|
||
| form.addEventListener('submit', (e) => { | ||
| e.preventDefault(); | ||
|
|
||
| const nameValue = form.elements.name.value.trim(); | ||
| const ageValue = form.elements.age.value.trim(); | ||
| const positionValue = form.elements.position.value.trim(); | ||
| const officeValue = form.elements.office.value; | ||
| const salaryValue = form.elements.salary.value; | ||
|
|
||
| if (!nameValue || !ageValue || !positionValue || !salaryValue) { |
There was a problem hiding this comment.
The code checks for empty positionValue in validation (line 95) but the input itself isn't required (line 68). This is inconsistent—either mark the input required or adjust validation. Prefer adding required to the Position input to satisfy checklist item #25 and HTML requirement.
| @@ -1,3 +1,151 @@ | |||
| 'use strict'; | |||
There was a problem hiding this comment.
Checklist item #1 requires the README/demo link to replace <your_account> with the GitHub username. This file doesn't address that; ensure your project README or description file is updated accordingly (not in this JS file).
| notification.className = isError ? 'error' : 'success'; | ||
| notification.textContent = message; |
There was a problem hiding this comment.
Notification creation uses className assignment (line 145) which overwrites any existing classes. If notification element needs both notification class and error/success, consider using classList.add to preserve classes. Also ensure notifications include visible title/description per checklist item #32 if tests expect it.
| rowsArray.sort((a, b) => { | ||
| const contentA = a.cells[colIndex].textContent; | ||
| const contentB = b.cells[colIndex].textContent; | ||
| const numA = parseFloat(contentA.replace(/[$,]/g, '')); | ||
| const numB = parseFloat(contentB.replace(/[$,]/g, '')); | ||
|
|
||
| let comparison = 0; | ||
|
|
||
| if (!isNaN(numA) && !isNaN(numB)) { | ||
| comparison = numA - numB; | ||
| } else { | ||
| comparison = contentA.localeCompare(contentB); |
There was a problem hiding this comment.
Sorting logic uses textContent and attempts numeric parse on every column. That may incorrectly treat numeric-looking strings (like ages) vs names. It's acceptable, but ensure column types align with expected behavior (checklist items #2-#4). Consider trimming content before comparing to avoid whitespace affecting localeCompare.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am requesting changes because there is still a critical failing test related to the position field validation and notification behavior. According to the Cypress logs, when an invalid value is entered in the position input, your code does not render a notification element with data-qa="notification", while similar flows for name and age do show the notification. This means a core requirement—showing an error notification for invalid form input—is not consistently met, and tests that rely on this behavior are failing, so the task cannot be considered complete yet.
To fix this, in src/scripts/main.js locate the form submission/validation block (around your existing name and age checks) and ensure the position validation branch both (1) checks the position value (e.g., length < 4 as you already do) and (2) calls the same notification-creation function you use for name/age errors, with data-qa="notification" on the element. Also confirm that the position input in the form has the selector the tests expect (for example data-qa="position" / data-qa="position-input", depending on the spec) and participates in the same validation flow as the other fields. Aside from this blocker, your sorting logic, row selection, form construction, and other validations (name, age, salary formatting, success/error notifications in general) look solid, and once you align the position error path with the others you should 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
|
|
||
| form.innerHTML = ` | ||
| <label>Name: <input name="name" type="text" data-qa="name" required></label> | ||
| <label>Position: <input name="position" type="text" data-qa="position" required></label> |
There was a problem hiding this comment.
The code correctly adds the required attribute to the Position input, which was the main previously reported blocker. Good — this matches the checklist item that all fields must be required.
| const rowsArray = Array.from(tbody.rows); | ||
|
|
||
| rowsArray.sort((a, b) => { | ||
| const contentA = a.cells[colIndex].textContent.trim(); |
There was a problem hiding this comment.
Sorting compares numeric values (salary/age) and text columns. Good use of trimming before comparison helps avoid whitespace affecting sort. Consideration: you already trim cell text during sorting — nice.
| activeRow.classList.remove('active'); | ||
| } | ||
|
|
||
| targetRow.classList.add('active'); |
There was a problem hiding this comment.
Row selection logic is correct: it ensures only one row has the active class at a time by removing it from the previously active row before adding to the clicked row.
| </select> | ||
| </label> | ||
| <label>Age: <input name="age" type="number" data-qa="age" required></label> | ||
| <label>Salary: <input name="salary" type="number" data-qa="salary" required></label> |
There was a problem hiding this comment.
Form markup includes correct fields, select options, qa attributes, and uses new-employee-form class as required. Age and salary inputs are numeric types and you convert salary to formatted string when adding to table — this matches the requirements.
| return; | ||
| } | ||
|
|
||
| if (positionValue.length < 4) { |
There was a problem hiding this comment.
Validation checks for required fields, name length, age range, and position length exist. However, the task requires showing error when Name has fewer than 4 letters and when Age is out of range — both are implemented. Note: you also validate positionValue.length < 4 (extra but acceptable).
| function showNotification(message, isError) { | ||
| const notification = document.createElement('div'); | ||
|
|
||
| notification.setAttribute('data-qa', 'notification'); |
There was a problem hiding this comment.
Notifications are created with data-qa="notification" and have error/success classes added via classList.add, which preserves other classes if present — this satisfies the requirement (and addresses an earlier suggestion).
| const rowsArray = Array.from(tbody.rows); | ||
|
|
||
| rowsArray.sort((a, b) => { | ||
| const contentA = a.cells[colIndex].textContent.trim(); |
There was a problem hiding this comment.
Potential improvement: when sorting textual columns, you already call trim() which is good. Consider also normalizing case (e.g., .toLowerCase()) to make text sorting case-insensitive as tests sometimes expect consistent ordering.
| document.body.appendChild(notification); | ||
|
|
||
| setTimeout(() => notification.remove(), 4000); | ||
| } |
There was a problem hiding this comment.
Minor: The implementation does not include optional double-click editing of table cells (cell-input creation, single editable cell, blur/Enter handling). If tests expect optional feature it's fine, but if required, it is missing.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am approving your solution as an educational override so you can keep progressing while you refine a few details.
You’ve done a strong job implementing the core requirements: table sorting with ASC/DESC behavior and reset on new column click, single-row selection via the active class, dynamic form creation with proper data-qa attributes, and notifications that include data-qa="notification" and error/success classes [CHECKLIST ITEMS #1-6, #7-9, #23-24, #30-34]. The notification logic and salary formatting (removing $ and commas for sorting, then formatting new salaries as currency) are also well thought out and aligned with the requirements [CHECKLIST ITEMS #3, #35-36]. The main issue to revisit is the position validation notification [CHECKLIST ITEM #38]: although you call showNotification('Position is too short', true); around lines 106-109 and showNotification correctly creates the notification element, earlier tests reported that no element with data-qa="notification" appeared for an invalid position.
As a next step, double-check that the position input’s attributes (data-qa="position" and name="position" around line 67) exactly match what the tests use, and consider aligning the error message/structure with your other validation notifications so they’re fully consistent. A small clarity improvement would be to replace form.setAttribute('novalidate', true); (line 63) with form.noValidate = true;, which better communicates intent and avoids string/boolean confusion, though it’s not functionally required. Overall, your implementation is solid and close to production-ready; with a small adjustment to the position error path and optional polish on form validation, you’ll have a very robust solution—nice work, and keep building on this foundation.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| if (positionValue.length < 4) { | ||
| showNotification('Position is too short', true); | ||
|
|
||
| return; |
There was a problem hiding this comment.
The position validation branch shows an error by calling showNotification('Position is too short', true); (lines 106-109), but the task's previous failing test indicated the notification element with data-qa="notification" was not rendered for invalid position. I verified that showNotification sets data-qa="notification" and appends the element (lines 138-151). Ensure the position validation is reached in the tests: check that the position input has the expected data-qa="position" attribute (line 67) and that the form uses name="position" (line 67) so form.elements.position resolves correctly. If tests still fail, they may expect the same notification structure (title/description) used for other flows—consider matching the message format used elsewhere. If you prefer, consolidate validation error calls into a single helper to guarantee consistent notification creation.
| const form = document.createElement('form'); | ||
|
|
||
| form.classList.add('new-employee-form'); | ||
| form.setAttribute('novalidate', true); |
There was a problem hiding this comment.
When setting novalidate via form.setAttribute('novalidate', true); (line 63) note that setAttribute always stores string values; this is fine for disabling HTML5 validation but if any test relies on presence check consider using form.noValidate = true;. Not required for functionality but can improve clarity.
| const numA = parseFloat(contentA.replace(/[$,]/g, '')); | ||
| const numB = parseFloat(contentB.replace(/[$,]/g, '')); | ||
|
|
||
| let comparison = 0; | ||
|
|
||
| if (!isNaN(numA) && !isNaN(numB)) { | ||
| comparison = numA - numB; | ||
| } else { | ||
| comparison = contentA.localeCompare(contentB); | ||
| } | ||
|
|
||
| return asc ? comparison : -comparison; | ||
| }); | ||
| tbody.append(...rowsArray); |
There was a problem hiding this comment.
Sorting: the code determines numeric sort by parsing cell text after removing $ and commas (lines 28-29) which works for the Salary column. Ensure the Salary column text always matches this format when new rows are added (you format salary on line 121). This satisfies checklist items about sorting behavior and salary formatting.
| tbody.addEventListener('click', (e) => { | ||
| const targetRow = e.target.closest('tr'); | ||
|
|
||
| if (!targetRow || !tbody.contains(targetRow)) { | ||
| return; | ||
| } | ||
|
|
||
| const activeRow = tbody.querySelector('.active'); | ||
|
|
||
| if (activeRow) { | ||
| activeRow.classList.remove('active'); | ||
| } | ||
|
|
||
| targetRow.classList.add('active'); | ||
| }); |
There was a problem hiding this comment.
Row selection logic (lines 44-58) correctly uses the active class and ensures only one row active at a time—this meets checklist items #4-#6. One minor edge: clicking table header could produce a tr from header area; your tbody.contains(targetRow) guard (line 47) prevents selecting header rows — good.
2 participants
https://dianakomar.github.io/js_employees_table_DOM/