Skip to content

Follow-up: fix reconfigure class migration edge cases - #3

Open
VishnuSivadasVS wants to merge 1 commit into
latestfrom
codex/analyze-and-fix-reported-issues-1qduv8
Open

Follow-up: fix reconfigure class migration edge cases#3
VishnuSivadasVS wants to merge 1 commit into
latestfrom
codex/analyze-and-fix-reported-issues-1qduv8

Conversation

@VishnuSivadasVS

Copy link
Copy Markdown
Member

Motivation

  • Reconfiguration after changing classNames left stale classes/selectors in the DOM and could produce duplicate toggle buttons or mis-targeted control cells.
  • Multi-row <thead> support needed consistent application of visibility and removal of only injected control cells.
  • Inline review flagged accidental insertion of control cells into generated details rows and duplicate delegated listeners across remounts.

Description

  • Add swapClassTokens(...) and _migrateStructureClasses(prevOptions, nextOptions) to migrate existing DOM classes for control/toggle/details elements when structural classNames change.
  • Avoid inserting control <td> into details rows by skipping rows with the details class in _ensureControlColumn() and track injected header <th> per header row with _insertedHeaderControls (a WeakSet) so destroy() removes only injected header controls.
  • Ensure event delegation handlers are only attached once per controller by guarding _clickHandler and _keyHandler, apply visibility to all header rows via headerRows, and add a _reconfigure(nextOptions, structural) path that uses isStructuralOptionsChange(...) to detect structural changes.
  • Rebuilt distribution artifacts and updated README/docs to reflect multi-row <thead> support and the above fixes.

Testing

  • Ran npm run build which completed successfully and produced updated dist/* artifacts.
  • Launched a lightweight docs server with python3 -m http.server 4173 and captured a browser render with a Playwright script to validate the docs page loaded after the build, which succeeded.
  • Performed automated code searches/inspections to verify the new helpers and reconfigure path exist in src/CollapseTable.js and the built bundles include the same changes.

Codex Task

@chatgpt-codex-connector chatgpt-codex-connector Bot 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.

💡 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".

Comment thread src/CollapseTable.js
Comment on lines +416 to +420
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);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge 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 👍 / 👎.

Comment thread src/CollapseTable.js
Comment on lines +424 to +425
this._insertedControl = true;
this._insertedHeaderControls.add(tr);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge 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 👍 / 👎.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

1 participant