add task solution#1113
Conversation
yana-longstocking
commented
Jun 2, 2026
yana-longstocking
commented
- DEMO LINK
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
There is a critical bug that must be fixed: when clicking a different column header, the sort direction is not reset to ASC as required by the specification. The global ascending variable persists across column clicks, causing unexpected DESC sorting when switching to a new column. To fix this, you need to track which column was last sorted and reset ascending = true when a different column is clicked. Additionally, the compareNumericValues and compareCurrencyValues functions receive an asc parameter but ignore it, using the global ascending instead—this inconsistency could cause bugs when these functions are called. Please implement per-column sort state tracking so that sorting a new column always starts in ASC order.
✨ 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 table = document.querySelector('table'); | ||
| let ascending = true; | ||
|
|
||
| table.onclick = function (e) { | ||
| if (e.target.tagName !== 'TH') { | ||
| return; | ||
| } | ||
|
|
||
| const th = e.target; | ||
|
|
||
| sortTable(th.cellIndex); | ||
| ascending = !ascending; |
There was a problem hiding this comment.
Bug: The ascending variable is not reset when clicking a new column header. According to the requirements, clicking a new title should always sort in ASC order. Currently, if you sorted one column DESC and then clicked a different column, it would sort DESC instead of ASC.
Fix approach: You need to track which column was last sorted. When a different column is clicked, reset ascending to true before sorting.
|
|
||
| function compareNumericValues(numA, numB, asc = true) { | ||
| if (ascending) { | ||
| return numA - numB; | ||
| } | ||
|
|
||
| return numB - numA; |
There was a problem hiding this comment.
The compareNumericValues function receives an asc parameter but ignores it, using the global ascending instead. This inconsistency could cause bugs when called from sortTable.
|
|
||
| function compareCurrencyValues(priceA, priceB, asc = true) { | ||
| const amountA = parseCurrency(priceA); | ||
| const amountB = parseCurrency(priceB); | ||
|
|
||
| if (ascending) { | ||
| return amountA - amountB; | ||
| } | ||
|
|
||
| return amountB - amountA; |
There was a problem hiding this comment.
The compareCurrencyValues function receives an asc parameter but ignores it, using the global ascending instead. Same inconsistency as numeric comparison.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Your solution is approved. All task requirements are met: table sorting correctly toggles between ASC/DESC and resets to ASC when switching columns (using lastSortedColumn tracking), compare functions properly use the asc parameter, row selection applies the 'active' class correctly, the employee form has the correct structure with all required data-qa attributes and 6 office options, form validation handles all required checks (name length, age range, required fields), notifications display with proper styling, salary formatting works correctly, and optional cell editing functions as expected. The critical bugs from previous reviews have been resolved. Great work implementing all features!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
2 participants
