add task solution#1268
Conversation
freelinex
commented
Feb 25, 2026
freelinex
commented
- DEMO LINK
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Good work on the core logic for adding and removing rows and columns! The event listeners correctly modify the table structure. However, this submission requires changes because a critical requirement is missing: the control buttons are not disabled when the row or column counts reach their limits.
Specifically, the buttons should be disabled when the count reaches the minimum (2) or maximum (10), and re-enabled when the count moves away from those limits. This is a core functional requirement to ensure the UI accurately reflects the state of the table.
A good approach to fix this would be to create a single function that checks the current row and column counts and updates the disabled property on all four buttons. This function should then be called after every add or remove operation.
✨ 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
| addRow.addEventListener('click', () => { | ||
| const count = table.childElementCount; | ||
|
|
||
| if (count < maxElements) { | ||
| tableRow.parentNode.append(createRow()); | ||
| } | ||
| }); | ||
|
|
||
| removeRow.addEventListener('click', () => { | ||
| const count = table.childElementCount; | ||
|
|
||
| if (count > minElements) { | ||
| table.lastElementChild.remove(); | ||
| } | ||
| }); | ||
|
|
||
| addColumn.addEventListener('click', () => { | ||
| const tr = document.querySelectorAll('.field tr'); | ||
|
|
||
| tr.forEach((element) => { | ||
| const count = element.cells.length; | ||
|
|
||
| if (count < maxElements) { | ||
| element.lastElementChild.after(createColumn()); | ||
| } | ||
| }); | ||
| }); | ||
|
|
||
| removeColumn.addEventListener('click', () => { | ||
| const tr = document.querySelectorAll('.field tr'); | ||
|
|
||
| tr.forEach((element) => { | ||
| const count = element.cells.length; | ||
|
|
||
| if (count > minElements) { | ||
| element.lastElementChild.remove(); | ||
| } | ||
| }); | ||
| }); |
There was a problem hiding this comment.
These event listeners correctly handle adding and removing elements. However, they are all missing the required logic to update the disabled state of the control buttons. After each operation, you need to check the current row and column counts and enable or disable the appropriate buttons when the count reaches the minimum (2) or maximum (10) limits.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Great job tackling the button state logic! However, this submission requires changes because the core functionality is not yet working as required, causing all the automated tests to fail.
The issue is that all four buttons (add/remove for rows and columns) are disabled one click too late. This happens because your code checks the element count before adding or removing an element. The requirement is to disable the button immediately after the action that causes the count to reach a limit (2 or 10).
To fix this, please adjust the logic in your event listeners:
- Perform the add or remove action first.
- Then, check the new row/column count.
- If the count has reached a limit, disable the corresponding button within the same event.
Applying this "act first, then check" pattern to all four buttons will get the tests passing. You're very close!
✨ 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
| } else { | ||
| addRow.setAttribute('disabled', ''); | ||
| } |
There was a problem hiding this comment.
This logic disables the 'Add Row' button one click too late. The button should be disabled immediately after the 10th row is added. Consider moving this check inside the if (count < maxElements) block, to be executed right after tableRow.parentNode.append(createRow()).
| } else { | ||
| removeRow.setAttribute('disabled', ''); | ||
| } |
There was a problem hiding this comment.
Similar to adding a row, this logic disables the button one click too late. The button should be disabled immediately after a row is removed and the count becomes 2. This check should be moved inside the if (count > minElements) block and executed after tbody.lastElementChild.remove().
| tr.forEach((element) => { | ||
| const count = element.cells.length; | ||
|
|
||
| if (count < maxElements) { | ||
| if (removeColumn.hasAttribute('disabled')) { | ||
| removeColumn.removeAttribute('disabled'); | ||
| } | ||
| element.lastElementChild.after(createColumn()); | ||
| } else { | ||
| addColumn.setAttribute('disabled', ''); | ||
| } | ||
| }); |
There was a problem hiding this comment.
The logic to check the column count and disable the button should be outside this loop to avoid redundant checks. More importantly, it suffers from the same 'one click too late' issue as the row buttons. Check the column count once before the loop, and then after adding columns to all rows, check if the new count has reached the maximum to disable the button immediately.
| tr.forEach((element) => { | ||
| const count = element.cells.length; | ||
|
|
||
| if (count > minElements) { | ||
| if (addColumn.hasAttribute('disabled')) { | ||
| addColumn.removeAttribute('disabled'); | ||
| } | ||
| element.lastElementChild.remove(); | ||
| } else { | ||
| removeColumn.setAttribute('disabled', ''); | ||
| } | ||
| }); |
There was a problem hiding this comment.
This logic has the same issues as addColumn. The count check should be done once outside the loop. The button disabling logic should be applied after the loop, checking if the new column count has reached the minimum of 2.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Great job getting the button disabling logic to work correctly! The application now behaves exactly as required, which is a big accomplishment.
I am approving this submission so you can move forward, but I want to highlight an important point from the previous feedback about the 'act first, then check' pattern. Your current code checks the number of rows/columns before adding or removing one. A more robust approach is to perform the action first (e.g., add the row) and then check the new state to update the buttons.
For example, in your addRow function:
- First, append the new row.
- Then, get the new total row count.
- Finally, use that new count to update the button states.
Applying this pattern to all four functions will make your code cleaner and easier to maintain. Keep up the excellent work!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| addRow.addEventListener('click', () => { | ||
| const count = tbody.childElementCount; | ||
|
|
||
| if (count === maxElements - 1) { | ||
| addRow.setAttribute('disabled', ''); | ||
| } | ||
|
|
||
| if (count < maxElements) { | ||
| if (removeRow.hasAttribute('disabled')) { | ||
| removeRow.removeAttribute('disabled'); | ||
| } | ||
| tableRow.parentNode.append(createRow()); | ||
| } | ||
| }); |
There was a problem hiding this comment.
This logic should be updated to follow the 'act first, then check' pattern. First, append the new row. Then, get the new total row count and use that to determine if the addRow button should be disabled or the removeRow button should be enabled.
| removeRow.addEventListener('click', () => { | ||
| const count = tbody.childElementCount; | ||
|
|
||
| if (count === minElements + 1) { | ||
| removeRow.setAttribute('disabled', ''); | ||
| } | ||
|
|
||
| if (count > minElements) { | ||
| if (addRow.hasAttribute('disabled')) { | ||
| addRow.removeAttribute('disabled'); | ||
| } | ||
| tbody.lastElementChild.remove(); | ||
| } | ||
| }); |
There was a problem hiding this comment.
Please apply the 'act first, then check' pattern here as well. The row should be removed first, and then the new count should be checked to update the states of the removeRow and addRow buttons.
| addColumn.addEventListener('click', () => { | ||
| const tr = document.querySelectorAll('.field tr'); | ||
|
|
||
| tr.forEach((element) => { | ||
| const count = element.cells.length; | ||
|
|
||
| if (count === maxElements - 1) { | ||
| addColumn.setAttribute('disabled', ''); | ||
| } | ||
|
|
||
| if (count < maxElements) { | ||
| if (removeColumn.hasAttribute('disabled')) { | ||
| removeColumn.removeAttribute('disabled'); | ||
| } | ||
| element.lastElementChild.after(createColumn()); | ||
| } | ||
| }); | ||
| }); |
There was a problem hiding this comment.
This should also be refactored to 'act first, then check'. The forEach loop should only handle adding the new column to each row. The logic for checking the column count and updating button states should happen once, after the loop has finished.
| removeColumn.addEventListener('click', () => { | ||
| const tr = document.querySelectorAll('.field tr'); | ||
|
|
||
| tr.forEach((element) => { | ||
| const count = element.cells.length; | ||
|
|
||
| if (count === minElements + 1) { | ||
| removeColumn.setAttribute('disabled', ''); | ||
| } | ||
|
|
||
| if (count > minElements) { | ||
| if (addColumn.hasAttribute('disabled')) { | ||
| addColumn.removeAttribute('disabled'); | ||
| } | ||
| element.lastElementChild.remove(); | ||
| } | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Please apply the 'act first, then check' pattern here too. The forEach loop should only remove the last column from each row. The check to update button states should be done once, after the loop.
2 participants
