Skip to content

Develop#1110

Open
galina-palyukh wants to merge 2 commits into
mate-academy:masterfrom
galina-palyukh:develop
Open

Develop#1110
galina-palyukh wants to merge 2 commits into
mate-academy:masterfrom
galina-palyukh:develop

Conversation

@galina-palyukh
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

Your solution successfully implements all core requirements from the task specification. Table sorting works correctly with ASC/DESC toggle, row selection applies the 'active' class properly, the form structure includes all required inputs with correct data-qa attributes, and form validation triggers notifications for Name (< 4 letters) and Age (< 18 or > 90) as specified. There are two issues worth noting for improvement: 1. Cell editing edge case: In the saveState() function, when editing a salary cell and submitting an empty value, the cell displays $0 instead of reverting to the original value. This happens because the salary formatting logic runs after the empty check, overwriting it. Moving the empty check after the salary formatting condition will fix this. 2. Redundant validation: The position field validation in checkValidation() is unnecessary since the HTML5 required attribute already prevents empty submissions for that field. These are minor issues that don't prevent task completion. The implementation is functional and meets all specified requirements.


✨ 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
Comment on lines +199 to +209

if (positionInput.value.length === 0) {
createNotification('error', 'The field "Position" cannot be empty');

return false;
}

if (ageInput.value < 18 || ageInput.value > 90) {
createNotification('error', 'Age must be between 18 and 90');

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 validation checks position field but the task only requires validation for Name (< 4 letters) and Age (< 18 or > 90). This extra validation may cause issues if the position field is intentionally left empty during testing.

Comment thread src/scripts/main.js
Comment on lines +284 to +295

const saveState = () => {
const value = input.value;

if (value === '') {
cell.textContent = cellOriginalValue;
} else {
cell.textContent = value;
}

if (columnName === 'salary') {
cell.textContent = `$${Number(value).toLocaleString('en-US')}`;
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Edge case: When cell is empty and column is 'salary', cell.textContent becomes '$NaN' instead of returning original value. The empty check should happen after the salary formatting condition.

Comment thread src/scripts/main.js
Comment on lines +193 to +194

if (nameInput.value.length < 4) {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Name validation doesn't trim whitespace - a name like 'abc ' (with trailing space) would incorrectly pass validation since it counts characters including spaces.

Comment thread src/scripts/main.js
Comment on lines +199 to +203

if (positionInput.value.length === 0) {
createNotification('error', 'The field "Position" cannot be empty');

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.

Position field validation (line 199) is not required by the task specification which only mentions Name (< 4 letters) and Age (< 18 or > 90) as validation rules.

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