Follow-up: fix reconfigure class migration edge cases#2
Follow-up: fix reconfigure class migration edge cases#2VishnuSivadasVS wants to merge 2 commits into
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e3f6519407
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const firstIsControl = first && hasAllClasses(first, this.options.classNames.control); | ||
| if (!firstIsControl) { | ||
| const td = createEl("td", this.options.classNames.control); | ||
| row.insertBefore(td, row.firstChild); |
There was a problem hiding this comment.
This change only injects a control <td> when a row’s first cell is missing the control class, but destroy() still deletes the first cell from every data row whenever _insertedControl is true. In tables where some rows already had author-provided control cells (or _insertedControl became true due header-row injection), calling unset()/destroy() will remove original row data from unaffected rows instead of restoring the initial DOM.
Useful? React with 👍 / 👎.
| for (const tr of this.headerRows) { | ||
| const firstCell = tr.cells[0]; | ||
| const firstIsControl = firstCell && hasAllClasses(firstCell, this.options.classNames.control); | ||
| if (!firstIsControl) { | ||
| const th = createEl("th", this.options.classNames.control); |
There was a problem hiding this comment.
Control header cells are now inserted for each header row, but visibility toggling still targets only this.headerRow.cells[0] in _updateTogglesVisibility(). With multi-row <thead> and wide layouts where anyHidden is false, only the first header-row control cell is hidden while other injected control headers remain visible, causing a stray leading header column and header/body misalignment.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a393312a41
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const buttons = Array.from(ctrlCell.querySelectorAll("button")); | ||
| if (buttons.length > 1) { | ||
| buttons.slice(1).forEach(btn => btn.remove()); | ||
| } |
There was a problem hiding this comment.
During structural reconfiguration, _migrateStructureClasses removes every button in the control cell except the first one, because it uses querySelectorAll("button") and blindly deletes buttons.slice(1). This will permanently remove author-provided secondary buttons (for example row action buttons) whenever class-related options are updated, even though those buttons are not duplicate collapse toggles.
Useful? React with 👍 / 👎.
| for (const tb of this.tbodies) { | ||
| for (const row of Array.from(tb.rows)) { | ||
| if (row.classList.contains(this.options.classNames.details)) continue; | ||
| if (!this._insertedBodyControls.has(row)) continue; |
There was a problem hiding this comment.
Teardown now deletes the first body cell only when the row is present in _insertedBodyControls, but rows that get a control cell later via _mountRowsInBody are never added to that WeakSet. If rows are appended while the table is mounted, destroy() skips removing those injected control cells and leaves the DOM structure mutated after unset().
Useful? React with 👍 / 👎.
1 participant
Motivation
classNamesleft stale classes/selectors in the DOM and could produce duplicate toggle buttons or mis-targeted control cells.<thead>support needed consistent application of visibility and removal of only injected control cells.Description
swapClassTokens(...)and_migrateStructureClasses(prevOptions, nextOptions)to migrate existing DOM classes for control/toggle/details elements when structuralclassNameschange.<td>into details rows by skipping rows with thedetailsclass in_ensureControlColumn()and track injected header<th>per header row with_insertedHeaderControls(aWeakSet) sodestroy()removes only injected header controls._clickHandlerand_keyHandler, apply visibility to all header rows viaheaderRows, and add a_reconfigure(nextOptions, structural)path that usesisStructuralOptionsChange(...)to detect structural changes.<thead>support and the above fixes.Testing
npm run buildwhich completed successfully and produced updateddist/*artifacts.python3 -m http.server 4173and captured a browser render with a Playwright script to validate the docs page loaded after the build, which succeeded.src/CollapseTable.jsand the built bundles include the same changes.Codex Task