Skip to content

add task solution#1089

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

add task solution#1089
JordanKoby wants to merge 2 commits into
mate-academy:masterfrom
JordanKoby:develop

Conversation

@JordanKoby
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 is approved because it implements all core requirements: sortable headers with correct ASC/DESC toggling, single-row selection via the active class, dynamic form creation with the right fields and data-qa attributes, validation and notifications, adding rows to the table, and optional cell editing.
In src/scripts/main.js, the sort comparator currently never returns 0 when valA and valB are equal and uses generic >/< checks; for more robust and test-friendly behavior, consider returning 0 for equal values and using localeCompare for string branches while keeping numeric comparison separate. Also in the sort logic, using !isNaN(valA) on the cleaned string means an empty string becomes 0 and is treated as a valid number; a safer pattern is to check valA.trim() === '' before Number(valA) or rely on Number.isNaN(Number(valA)) to distinguish empties from numbers.
In your form submission handler, the salary presence check with if (!salary) can mis-treat an explicit 0 as missing and also conflates empty input with numeric zero after conversion; validate using the raw form value (e.g. data.salary) or Number.isFinite(Number(rawSalary)) so you correctly detect missing input versus 0.
For the optional cell-editing feature, you currently allow multiple cells to be edited at once; to better match the “only one editable cell” requirement, you can look for an existing document.querySelector('.cell-input'), save/close it, and only then insert a new input into the newly double-clicked cell.
These issues are mostly robustness and edge-case concerns rather than core feature failures, but addressing them will make your sorting and validation behavior more reliable and closer to production quality.
Overall, you’ve met the main functional goals of the task, and with the above refinements your implementation will be even stronger and easier to maintain.


✨ 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 Outdated
valB = Number(valB);
}

return asc ? (valA > valB ? 1 : -1) : valA < valB ? 1 : -1;
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 comparator always returns 1 or -1, even when values are equal. This can produce incorrect/unstable ordering. For strings consider using localeCompare and in any case return 0 when values are equal. Also handle string vs number branches separately to keep logic clear.

Comment thread src/scripts/main.js
Comment on lines +20 to +25
let valA = a.children[index].innerText.replace(/[$,]/g, '');
let valB = b.children[index].innerText.replace(/[$,]/g, '');

if (!isNaN(valA) && !isNaN(valB)) {
valA = Number(valA);
valB = Number(valB);
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Using !isNaN(valA) on the raw string after removing $/, can treat an empty string ('') as numeric (Number('') === 0). That causes empty cells to be treated as 0 during numeric sort. Consider using Number.isNaN(Number(valA)) or checking valA.trim() === '' before converting to Number.

Comment thread src/scripts/main.js
Comment on lines +134 to +141
if (cell.tagName === 'TD' && !cell.querySelector('input')) {
const oldValue = cell.innerText;
const input = document.createElement('input');

input.value = oldValue;
input.className = 'cell-input';
cell.innerHTML = '';
cell.appendChild(input);
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Optional requirement: only one cell should be editable at a time. Current code only checks the target cell for an existing input but doesn't close any other open .cell-input. Consider checking document.querySelector('.cell-input') and saving/closing it before inserting a new input.

Comment thread src/scripts/main.js Outdated
Comment on lines +110 to +113
if (!salary) {
showNotification('Salary is required', 'error');

return;
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 salary presence check if (!salary) relies on the numeric value and treats 0 (and '' converted to 0) as falsy. Validate using the raw FormData value (e.g. check data.salary is not empty) or use Number.isFinite(salary)/isNaN to distinguish missing input from numeric zero.

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