Follow-up: fix reconfigure class migration edge cases#3
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8f27a00e9c
ℹ️ 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".
| 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.
Toggle control-column visibility on all header rows
This block now injects a control <th> into every THEAD row, but _updateTogglesVisibility() still only toggles this.headerRow.cells[0]. In multi-row headers when no data columns are hidden, secondary header rows keep a visible control cell while body control cells are hidden, leaving an orphan leading column and inconsistent header/body alignment.
Useful? React with 👍 / 👎.
| this._insertedControl = true; | ||
| this._insertedHeaderControls.add(tr); |
There was a problem hiding this comment.
Track body insertions before deleting first data cells
Setting _insertedControl when only a header row receives an injected control cell makes destroy() remove the first cell from every data row even if those body cells were never injected by CollapseTable. In multi-row tables where only some header rows needed a synthesized control <th>, calling unset() can delete user-authored first-column data.
Useful? React with 👍 / 👎.
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