fix: render text alignment from rich-text editor end-to-end (#1201)#1396
fix: render text alignment from rich-text editor end-to-end (#1201)#1396nanangdev wants to merge 3 commits into
Conversation
…rough ProseMirror ↔ Portable Text round-trip (emdash-cms#1201)
…ms#1201) The previous fix in text-align-round-trip patched only the packages/core/src/content/converters/ pair but the rich-text editor saves through two other ProseMirror to Portable Text converters that each carried their own copy of the same logic - so reporter and maintainer kept seeing alignment dropped on save: - packages/admin/src/components/PortableTextEditor.tsx (admin save path) - packages/core/src/components/InlinePortableTextEditor.tsx (in-page editing) Both now forward node.attrs?.textAlign for paragraphs and headings (only center, right, justify - left is the editor default and is omitted so existing content stays byte-identical) and restore it on the reverse path. Each editor's local PortableTextTextBlock interface gained the textAlign?: "left" | "center" | "right" | "justify" field, mirroring the type in packages/core/src/content/converters/types.ts. The Portable Text frontend renderer now emits a WordPress-style has-text-align-{value} class on the rendered <p> / <h1..h6> / <blockquote> whenever the block carries textAlign. A new Block component is added under emdash/ui for callers composing custom Portable Text components who want to keep the EmDash behaviour. The class allowlist is enforced via Object.hasOwn, so a hostile or hand-edited Portable Text block cannot inject arbitrary class names. Consolidating the three duplicated converter pairs into a single shared module is a follow-up refactor and intentionally out of scope here. Closes emdash-cms#1201
🦋 Changeset detectedLatest commit: d8b3f7b The changes in this PR will be included in the next version bump. This PR includes changesets to release 14 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 |
There was a problem hiding this comment.
This PR correctly diagnoses and fixes the end-to-end text-alignment issue described in #1201. The root cause — three duplicated ProseMirror ↔ Portable Text converter pairs, only one of which was patched in an earlier attempt — is real, and the approach of patching all three pairs plus adding a frontend Block override is sound and idiomatic for EmDash. The omit-on-default rule for left is consistent across all converters, preserving existing content byte-for-byte. The Object.hasOwn class-name allowlist in textAlignClassName is a correct security choice.
I found one blocking contract issue and one cross-cutting observation:
-
The new
Blockcomponent is not exported fromemdash/ui. The PR description says it is, butpackages/core/src/ui.tsdoes not re-export it from./components/index.js. Consumers who want to compose custom Portable Text component sets while keeping EmDash's text-align behaviour have no public import path.ui.tsalready exportstype Blockfromastro-portabletext; adding the componentBlockalongside it is valid TypeScript (value and type exports can share a name). -
Blockquote and list-item alignment are not round-tripped. This is a pre-existing limitation, not a regression introduced here. The new
Blockrenderer does emithas-text-align-*on<blockquote>, and TipTap's TextAlign can apply to paragraphs inside list items, but none of the three converter pairs preservetextAlignon paragraphs inside blockquotes or on list-item blocks. Worth a follow-up issue since the visual gap will now be more noticeable.
The core converter round-trip tests and the textAlignClassName unit tests are well written. Lint and typecheck are clean on the changed files (unverified locally due to no node_modules, but the patterns are correct and the author provided passing results).
The PR description claimed the new Block component was exported from emdash/ui for callers composing custom Portable Text component sets, but ui.ts only re-exported the existing block-type components and never the new Block style override. Add it alongside the type-only Block re-export from astro-portabletext — value and type can share a name under verbatimModuleSyntax. Caught by emdashbot review on PR emdash-cms#1396.
|
Thanks for the careful read, @emdashbot. 1. 2. Blockquote and list-item alignment. Confirmed pre-existing. The TipTap The new |
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. |
What does this PR do?
The bot's earlier fix in
text-align-round-trippatched only thepackages/core/src/content/converters/pair, but the rich-text editor saves through two other ProseMirror ↔ Portable Text converters that each carry their own copy of the same logic. That is why @kodster28's June 2 reply said alignment still didn't show on previews or published versions, and why the bot's second attempt was rejected — the converter unit tests were green in isolation, but the admin save path was unfixed.This PR finishes #1201 end-to-end:
1. Admin save path —
packages/admin/src/components/PortableTextEditor.tsxThe inline
prosemirrorToPortableTextandportableTextToProsemirrornow forwardnode.attrs?.textAlignfor paragraphs and headings, and restore it on the reverse path. The localPortableTextTextBlockinterface gains thetextAlign?: "left" | "center" | "right" | "justify"field.2. In-page inline editor —
packages/core/src/components/InlinePortableTextEditor.tsxSame patch shape applied to
_pmToPortableText/_portableTextToPMand the localPTTextBlockinterface, so visual-editing mode round-trips alignment too.3. Public Portable Text renderer —
packages/core/src/components/Block.astro+index.tsA new EmDash
Blockoverride forastro-portabletextemits a WordPress-stylehas-text-align-{value}class on the rendered<p>/<h1..h6>/<blockquote>whenever the block carriestextAlign.leftis the editor default and intentionally does not produce a class — paragraphs/headings without explicit alignment render byte-identically to the previous output. The class allowlist is enforced viaObject.hasOwn, so a hostile or hand-edited Portable Text block cannot inject arbitrary class names. The newBlockis also exported fromemdash/uifor callers composing custom Portable Text components who want to keep the EmDash behaviour.In all three converters, only
center,right, andjustifyare persisted —leftand any other value are omitted, matching the omit-on-default rule the bot used in the core converter pair so existing content stays byte-for-byte unchanged.Consolidating the three duplicated converter pairs into a single shared module is a follow-up refactor and intentionally out of scope here.
Closes #1201
Type of change
Checklist
pnpm typecheckpasses (verified viatsgo --noEmitonpackages/coreandpackages/adminafter building workspace deps)pnpm lintpasses (oxlint clean — 0 diagnostics on changed files)pnpm testpasses (targeted: 6 new + 33 existing component/converter tests green; full suite not run locally — see note below)pnpm formathas been run (oxfmt + prettier clean on changed files)emdashpatch — fixed-group bumps@emdash-cms/adminand the rest automatically)AI-generated code disclosure
Screenshots / test output
Local verification on Node 24.7.0, pnpm 11.1.3:
New regression test:
Targeted suite — components + converters (covers all three patched converter pairs and the new
Blockhelper):Visual end-to-end on
demos/simpleafter rebuildingpackages/admin:The same post that previously rendered as
now renders as
DB inspection (
SELECT content FROM ec_posts WHERE ...) confirms the corresponding revision rows now persisttextAlign: "center" | "right"on the affected blocks; left-aligned blocks remain free of the field, so existing seed content is unchanged.Lint:
Typecheck:
Note on full suite: I did not run
pnpm testacross the whole monorepo locally — the suite (3,761 tests inpackages/corealone, plus admin/blocks/etc.) hits 5+ minutes in this sandbox and was timing out the runner. The targeted suite I did run includes every test file that touches the patched converters (tests/unit/components/inline-portable-text-*,tests/unit/converters/text-align-round-trip.test.ts, the four other converter round-trip tests, and the newportable-text-text-align-class.test.ts). CI will run the full suite.Out-of-scope follow-up: the three duplicated
prosemirrorToPortableText/portableTextToProsemirrorpairs (inpackages/core/src/content/converters/,packages/admin/src/components/PortableTextEditor.tsx, andpackages/core/src/components/InlinePortableTextEditor.tsx) carry the same logic. This bug recurred precisely because patching one location left the other two stale. Happy to open a separate issue/PR to consolidate them behind a publishedemdash/converterssubpath if the maintainers want to schedule it.