Skip to content

version 1#1303

Open
dianakomar wants to merge 3 commits into
mate-academy:masterfrom
dianakomar:develop
Open

version 1#1303
dianakomar wants to merge 3 commits into
mate-academy:masterfrom
dianakomar:develop

Conversation

@dianakomar

Copy link
Copy Markdown

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Review Summary

I am requesting changes because some behaviors around button initialization and column counting can break the required min/max disabling behavior and may cause tests to fail.

  1. In src/scripts/main.js, in the initialization block where you compute rowsCount and colsCount (around the top of the file, near where you query the buttons), you currently only set btnAppendRow.disabled; instead, call your updateButtons() function once there so that all four buttons start in the correct enabled/disabled state according to the current rows/cols (this is essential for the requirement that buttons are disabled at min/max on initial load).
  2. In the row-removal handler in src/scripts/main.js (where you check whether a row can be removed), you use a DOM length (rows.length) while also tracking rowsCount; to avoid desynchronization between your counter and the DOM, base the condition on rowsCount (e.g., if (rowsCount > minCount)) and update the DOM and rowsCount together whenever you add or remove rows.
  3. When you initialize colsCount from the first row (the logic using firstRow.querySelectorAll('td')), this will return 0 if the first row uses <th> header cells; change this to firstRow.querySelectorAll('td, th').length so the initial colsCount correctly reflects all visible columns.
    As minor improvements, it would also be safer to consistently operate on tbody tr rather than all tr elements (so header rows are never modified) and to rely solely on updateButtons() instead of any one-off disabled assignments, keeping the logic more maintainable.
    Your core add/remove row/column functionality and min/max guards are otherwise solid, and with these adjustments your solution will fully match the intended behavior and be more robust.

✨ 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

  1. Rate AI review example

  2. Re-request AI review example

  3. Add AI review comments example

Comment thread src/scripts/main.js Outdated
}
updateButtons();
});
btnAppendRow.disabled = rowsCount >= maxCount;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

You only set btnAppendRow.disabled here which leaves the other buttons' initial states undefined. Call updateButtons() once after initialization (instead of this single assignment) to correctly set disabled/enabled states for all four buttons according to current rows/cols, per the requirement to disable buttons at min/max.

Comment thread src/scripts/main.js Outdated
btnRemoveRow.addEventListener('click', (e) => {
const rows = document.querySelectorAll('tr');

if (rows.length > minCount) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This check uses DOM rows.length while the code tracks rowsCount separately. For consistency and to avoid desync between the counter and DOM, use rowsCount in the condition (e.g. if (rowsCount > minCount)) and update the DOM and the counter together.

Comment thread src/scripts/main.js Outdated
const table = document.querySelector('table');
let rowsCount = document.querySelectorAll('tr').length;
const firstRow = document.querySelector('table tr');
let colsCount = firstRow ? firstRow.querySelectorAll('td').length : 0;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

colsCount is computed using firstRow.querySelectorAll('td'). If the first row uses header cells (th) this will return 0 and produce an incorrect initial columns count. Consider using firstRow.querySelectorAll('td, th').length so the initial colsCount reflects header cells as well.

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Copy link
Copy Markdown

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 correctly implements the required growth-table behavior: rows and columns can be added and removed, the 2–10 min/max limits are enforced for both, and the button disabled states are consistently updated on initialization and after each change. The logic in src/scripts/main.js keeps the internal rowsCount/colsCount counters in sync with the DOM, which avoids desynchronization bugs when removing rows or columns. There are no critical issues that would block functionality or violate the task checklist.

As minor improvements, you could refine the column-append handler in src/scripts/main.js so that when it processes the header row (the one containing <th> cells), it appends a <th> instead of a <td> to keep semantic HTML consistent. Similarly, instead of using document.querySelectorAll('tr') in your handlers, prefer table.querySelectorAll('tbody tr') so that body operations don’t accidentally touch header rows and to keep the scope limited to the relevant table. You might also consider extracting a small helper like createCell(isHeader) to centralize cell creation and reduce duplication between row and column operations. Overall, this is a solid, clear implementation that fulfills the functional requirements; the suggestions above are just to improve maintainability and HTML semantics.


✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨

Footnotes

  1. Rate AI review example

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