Skip to content

feat: support explicit nested cell options - #134

Open
tecfu wants to merge 3 commits into
masterfrom
feat/explicit-cell-options
Open

tecfu wants to merge 3 commits into
masterfrom
feat/explicit-cell-options

Conversation

@tecfu

@tecfu tecfu commented Oct 6, 2026

Copy link
Copy Markdown
Owner

Add an unambiguous { value, options } cell shape while preserving the existing flat cell-object syntax. This separates cell payload from rendering configuration and gives callers a safer shape to build programmatically.

@tecfu

tecfu commented Oct 6, 2026

Copy link
Copy Markdown
Owner Author

Review: merge (two notes)

Clean, minimal, additive. The one-line change in buildCell composes correctly with everything that landed recently, and I verified the interactions specifically:

  • With perf: share merged cell option bases per column #111's shared option bases: Object.assign(Object.create(base), elem, elem.options || {}) keeps writes per-cell (own props on the child) — no leak into the column base. ✅
  • With fix: cell functions and formatters run once per render, not twice #123's cell memo: { value: fn, options } executes fn exactly once per render (measured with a counting probe). ✅
  • Header rows with options (e.g. { value: "hdr", options: { align: "right" } }) render correctly. ✅
  • Flat legacy cell objects unchanged: matrix 0 diff, 21/21 goldens, 111/111 vitest, typecheck + lint clean.

Note on the flat-vs-nested precedence: elem.options is assigned after elem, so on a conflict { value, align: "left", options: { align: "right" } } → right wins (nested wins). That's the sane rule but it's invisible in the code and undocumented.

1. The README (Header and cell examples sections) doesn't mention the nested shape at all, and #125 made types public — a paragraph here (or a line in types.ts's Cell jsdoc beyond the interface) is where users will look.
2. Consider a test for the conflict-precedence case above, so the rule is pinned.

Sincerely qwen3.8-flash-next

tecfu commented Oct 6, 2026

Copy link
Copy Markdown
Owner Author

Review: merge, with two documentation/test notes

The nested { value, options } shape is implemented with a small, well-contained change: legacy flat cell objects continue to work and nested options are applied after the cell's own properties, giving nested options an unambiguous precedence.

The main remaining gap is that the public contract is not documented. The new Cell type is exported, but the README does not show the nested form or explain precedence when both the flat and nested forms specify the same option.

I would also add a regression test for the precedence rule, for example { value: 42, align: left, options: { align: right } }, so the intentionally chosen behavior cannot drift.

I do not see a merge-blocking implementation defect.

— GPT-5.6 Luna

tecfu commented Oct 7, 2026

Copy link
Copy Markdown
Owner Author

Re-review: approved

Nested cell options have clear precedence over legacy flat properties, and the regression now locks that precedence down explicitly.

CI is running on the updated test. No remaining implementation blocker found.

— GPT-5.6 Luna

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.

1 participant