feat(admin): add table support to PortableText editor#921
Conversation
🦋 Changeset detectedLatest commit: 0ac7f82 The changes in this PR will be included in the next version bump. This PR includes changesets to release 13 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Scope checkThis PR changes 755 lines across 7 files. Large PRs are harder to review and more likely to be closed without review. If this scope is intentional, no action needed. A maintainer will review it. If not, please consider splitting this into smaller PRs. See CONTRIBUTING.md for contribution guidelines. |
@emdash-cms/admin
@emdash-cms/auth
@emdash-cms/blocks
@emdash-cms/cloudflare
emdash
create-emdash
@emdash-cms/gutenberg-to-portable-text
@emdash-cms/x402
@emdash-cms/plugin-ai-moderation
@emdash-cms/plugin-atproto
@emdash-cms/plugin-audit-log
@emdash-cms/plugin-color
@emdash-cms/plugin-embeds
@emdash-cms/plugin-forms
@emdash-cms/plugin-webhook-notifier
commit: |
Overlapping PRsThis PR modifies files that are also changed by other open PRs:
This may cause merge conflicts or duplicated work. A maintainer will coordinate. |
|
/review |
|
Failed to check permissions for user ascorbic: HttpError: Requires authentication - https://docs.github.com/rest |
|
@ascorbic Bonk workflow failed. Check the logs for details. View workflow run · To retry, trigger Bonk again. |
| > | ||
| <BubbleButton | ||
| onClick={() => editor.chain().focus().addColumnBefore().run()} | ||
| title="Add column before" |
| </BubbleButton> | ||
| <BubbleButton | ||
| onClick={() => editor.chain().focus().addColumnAfter().run()} | ||
| title="Add column after" |
There was a problem hiding this comment.
Also this and all the others
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Nice work porting #222 forward — the PT↔PM round-trip looks correct, per-cell markDefs lines up with the existing Table.astro renderer in @emdash-cms/blocks/core, and the test coverage of round-tripping is solid.
A couple of things to address before merge:
- Localization (echoing @ascorbic). Per AGENTS.md ("All admin UI strings, aria labels, and toast messages go through Lingui"), the new
titleprops on the table bubble menu and toolbar button should be wrapped witht..., since `BubbleButton`/`ToolbarButton` propagate `title` to `aria-label` (see `BubbleButton` at line 2579). Yes, the existing surrounding buttons are also unwrapped — but new strings should not extend that pattern; they're untranslated forever once shipped. The slash-command entry already uses `msg`..., so just match that. - Version pinning in
pnpm-workspace.yaml. The four new@tiptap/extension-table*entries are pinned to exact3.20.0while every other@tiptap/*entry uses^3.20.0. This will diverge on the next TipTap patch bump and is almost certainly unintentional.
Non-blocking observations inline.
| "@tiptap/extension-table": 3.20.0 | ||
| "@tiptap/extension-table-cell": 3.20.0 | ||
| "@tiptap/extension-table-header": 3.20.0 | ||
| "@tiptap/extension-table-row": 3.20.0 |
There was a problem hiding this comment.
These four are pinned to exact 3.20.0, but every other @tiptap/* catalog entry uses ^3.20.0 (see e.g. @tiptap/extension-link, @tiptap/extension-placeholder, @tiptap/extension-text-align just above and below). On the next TipTap patch release the table extensions will fall behind the rest of the toolchain, which can break the peer-dep linkage with @tiptap/core/@tiptap/pm.
| "@tiptap/extension-table-row": 3.20.0 | |
| "@tiptap/extension-table": ^3.20.0 | |
| "@tiptap/extension-table-cell": ^3.20.0 | |
| "@tiptap/extension-table-header": ^3.20.0 | |
| "@tiptap/extension-table-row": ^3.20.0 |
| > | ||
| <BubbleButton | ||
| onClick={() => editor.chain().focus().addColumnBefore().run()} | ||
| title="Add column before" |
There was a problem hiding this comment.
Per AGENTS.md these need to go through Lingui — BubbleButton forwards title to both the native tooltip and aria-label, so this string is read by screen readers. Same applies to every title="..." literal in this new TableBubbleMenu (lines 2505, 2512, 2519, 2528, 2535, 2540, 2549, 2553) and the new toolbar entry at line 2836 (title="Insert Table").
This component already uses useLingui (see line 1884), so:
| title="Add column before" | |
| title={t`Add column before`} |
and similar for the others. The slash-command entry above (line 935) already uses msg...``, so the i18n pattern for this PR is established — just propagate it to the buttons.
|
|
||
| <BubbleButton | ||
| onClick={() => editor.chain().focus().toggleHeaderRow().run()} | ||
| active={editor.isActive("tableHeader")} |
There was a problem hiding this comment.
Minor UX note (non-blocking): editor.isActive("tableHeader") reflects whether the cursor is currently inside a <th>, not whether the table has a header row. So the toggle button will look "off" whenever the user is in a body cell — even on a table that does have a header row. If you want the indicator to track the table's header-row state, you'd need to walk the table node and check if its first row contains any tableHeader cells. Fine to leave for a follow-up.
| _type: "table", | ||
| _key: tableKey, | ||
| rows, | ||
| hasHeaderRow: rows[0]?.cells.some((cell) => cell.isHeader) ?? false, |
There was a problem hiding this comment.
hasHeaderRow is derived as "row 0 has any header cell". Combined with the read path at line 667 (cell.isHeader || (tableBlock.hasHeaderRow && rowIndex === 0)), a table where row 0 is a mix of <th> and <td> will round-trip with the entire row promoted to headers on the next load. Probably OK — toggleHeaderRow flips the whole row in TipTap — but worth noting if anyone later adds toggleHeaderCell to the bubble menu.
| import { | ||
| _portableTextToProsemirror, | ||
| _prosemirrorToPortableText, | ||
| } from "../../src/components/PortableTextEditor"; |
There was a problem hiding this comment.
These are pure conversion-function tests — nothing here actually exercises a browser environment. Worth pulling them out of the browser test target so they run in the cheap node suite (the PR description notes the file is currently run with --browser.enabled=true). Non-blocking.
|
Review posted successfully. Summary:
|
Lunaria Status Overview🌕 This pull request will trigger status changes. Learn moreBy default, every PR changing files present in the Lunaria configuration's You can change this by adding one of the keywords present in the Tracked Files
Warnings reference
|
jcheese1
force-pushed
the
codex-table-support
branch
from
393875f to
0ac7f82
Compare
ascorbic
left a comment
ascorbic
left a comment
There was a problem hiding this comment.
Thanks for the additions!
2 participants
What does this PR do?
Adds table support to the admin PortableText editor, porting the abandoned work from #222 and adjusting mark definition storage so authored tables render with the existing table renderer.
Discussion: #217
Changes:
/tableslash-command entry and toolbar button._type: "table"blocks.markDefson the cell payload so links render through the existing@emdash-cms/blockstable renderer.Closes #222
Type of change
Checklist
pnpm typecheckpassespnpm lintpassespnpm testpasses (or targeted tests for my change)pnpm formathas been runmessages.pochanges except in translation PRs — a workflow extracts catalogs on merge tomain.AI-generated code disclosure
Screenshots / test output
Verified:
pnpm formatpnpm --silent lint:quickpnpm --filter @emdash-cms/admin typecheckpnpm --dir packages/admin exec vitest run --browser.enabled=true tests/components/PortableTextEditor.table.test.tsNote: full
pnpm --silent lint:json | jq '.diagnostics | length'currently reports existing diagnostics outside the files touched by this PR. The changed-file quick lint is clean.