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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant