fix(core): make editor text alignment render on public pages - #2410
Conversation
🦋 Changeset detectedLatest commit: 45e8bf4 The changes in this PR will be included in the next version bump. This PR includes changesets to release 17 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 |
@emdash-cms/admin
@emdash-cms/auth
@emdash-cms/auth-atproto
@emdash-cms/blocks
@emdash-cms/cloudflare
@emdash-cms/contentful-to-portable-text
emdash
create-emdash
@emdash-cms/gutenberg-to-portable-text
@emdash-cms/plugin-cli
@emdash-cms/plugin-types
@emdash-cms/registry-client
@emdash-cms/registry-lexicons
@emdash-cms/registry-verification
@emdash-cms/sandbox-workerd
@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-field-kit
@emdash-cms/plugin-forms
@emdash-cms/plugin-webhook-notifier
commit: |
![emdashbot[bot]](https://avatars.githubusercontent.com/in/3255304?s=60&v=4){kind=small}
There was a problem hiding this comment.
The approach is sound: the Block override already emits has-text-align-* classes, so shipping scoped CSS for them is the right, minimal fix for the public rendering gap. Keeping the left alignment class-less and using Astro's scoped <style> matches the existing component patterns (Code.astro, Table.astro, Columns.astro) and avoids restyling arbitrary user markup. The changeset targets the correct package (emdash).
What I checked: the diff, Block.astro, the textAlignClassName helper, PortableText.astro/component exports, sibling components' scoped-style pattern, the new test, and the existing repro tests.
Headline conclusion: the fix is correct, but there are three AGENTS.md/quality issues to address before merge:
- Comment discipline in
Block.astro— the new JSX comment justifies the scoping choice and explains whylefthas no rule. That context belongs in the commit message/PR description, not in the component. - Comment discipline in the new test — a large JSDoc block summarises the test strategy and Astro internals, and the
describestring references#2285. - The test asserts an implementation proxy, not the behavior — it checks that Astro's
data-astro-cid-*scoping attribute exists, which only proves a scoped<style>tag exists, not that the style rules themselves are present or correct. A regression that deletes or mistypes the actual CSS could still pass.
Fixing these keeps the diff clean and the test actually meaningful.
| {/* Converting this to `is:global` would restyle any `has-text-align-*` | ||
| markup the site renders outside Portable Text. `left` maps to no class and | ||
| needs no rule. */} |
There was a problem hiding this comment.
[needs fixing] This JSX comment justifies the decision to use scoped styles instead of is:global and explains why left has no rule. Per AGENTS.md, comments should not justify decisions, narrate rejected alternatives, or restate what the code plainly shows (left is visibly absent from the selectors). Remove the comment.
| {/* Converting this to `is:global` would restyle any `has-text-align-*` | |
| markup the site renders outside Portable Text. `left` maps to no class and | |
| needs no rule. */} | |
| } | |
| <style> | |
| .has-text-align-center { |
| /** | ||
| * Renders PortableText through the container API and pins that the | ||
| * `has-text-align-{value}` classes emitted by the Block override come with | ||
| * the CSS to back them. Astro stamps its scoping attribute | ||
| * (`data-astro-cid-*`) on template elements only when the component | ||
| * carries a scoped `<style>`, so the attribute next to the alignment class | ||
| * is the signal that the rules ship and target the aligned element. | ||
| */ |
There was a problem hiding this comment.
[needs fixing] This JSDoc is a summary of the test strategy and Astro internals — exactly the kind of narrative AGENTS.md says comments should not contain. The test name and assertions should speak for themselves. Delete the block comment.
| /** | |
| * Renders PortableText through the container API and pins that the | |
| * `has-text-align-{value}` classes emitted by the Block override come with | |
| * the CSS to back them. Astro stamps its scoping attribute | |
| * (`data-astro-cid-*`) on template elements only when the component | |
| * carries a scoped `<style>`, so the attribute next to the alignment class | |
| * is the signal that the rules ship and target the aligned element. | |
| */ | |
| import PortableText from "../../src/components/PortableText.astro"; | |
| function alignedBlock(key: string, textAlign: string | undefined, text: string) { |
| return html.match(/<p\b[^>]*>/g) ?? []; | ||
| } | ||
|
|
||
| describe("PortableText text-align CSS (#2285)", () => { |
There was a problem hiding this comment.
[needs fixing] AGENTS.md forbids issue/PR references in code comments. A describe label is effectively reader-facing text in the suite; the issue link belongs in the PR description and commit message, not the test output. Drop #2285.
| describe("PortableText text-align CSS (#2285)", () => { | |
| describe("PortableText text-align CSS", () => { |
| const [p] = paragraphTags(html); | ||
|
|
||
| expect(p).toContain(`has-text-align-${align}`); | ||
| expect(p).toMatch(/data-astro-cid-/); |
There was a problem hiding this comment.
[needs fixing] This assertion tests an Astro implementation detail (the scoping attribute exists) as a proxy for "CSS ships and targets the element." It cannot catch regressions that remove or mistype the actual .has-text-align-{align} { text-align: ... } rules while leaving an empty scoped <style> in place. Replace this with an assertion on the emitted CSS itself if renderToString returns the style tag, or mount the output and assert getComputedStyle(paragraph).textAlign via a lightweight DOM fixture.
| expect(p).toMatch(/data-astro-cid-/); | |
| const styleBlock = html.match(/<style[^>]*>([\s\S]*?)<\/style>/)?.[1] ?? ""; | |
| expect(styleBlock).toContain(`.has-text-align-${align}`); | |
| expect(styleBlock).toContain(`text-align: ${align}`); |
(Adjust if your Astro container setup extracts styles rather than inlining them.)
There was a problem hiding this comment.
Agreed on the substance, and I moved the claim to a level that can carry it. Both mechanics you suggested turned out to be unavailable here, so this took a different route.
The container API emits no CSS at all. renderToString on this component returns only the paragraph markup:
<p class="has-text-align-center" data-astro-cid-…>Aligned</p>There is no <style> tag to match, so a regex over the output would assert against an empty string and pass unconditionally. Importing the compiled style module directly (Block.astro?astro&type=style&index=0&lang.css) resolves to "" under vitest.repro.config.ts, and getComputedStyle needs styles the container never produces.
So the repro test now only claims what it can prove — that the Block override emits has-text-align-{align} for a block's textAlign — and the CSS claim moved to an e2e spec that loads a page and reads the computed alignment from the browser. That covers the mistyped-rule case you raised, and also the case neither test saw before: whether the scoped style reaches a rendered page at all.
The spec renders its own Portable Text value from a fixture page rather than a seeded entry, to keep the suite's shared content — and the admin visual-regression baselines — untouched.
Verified on both e2e targets: 4/4 green, and 3/4 red with Block.astro restored to its state on main, failing on the alignment assertion itself.
The repro test asserted Astro's `data-astro-cid-*` scoping attribute as a stand-in for "the alignment rules ship and apply". That proxy cannot fail on the regression it guards: the attribute is stamped whenever the component carries any scoped style, so deleting or mistyping the rules keeps it green. The container API cannot do better — `renderToString` emits markup only, and the compiled style module resolves to an empty string under the repro config. Move the CSS claim to the level that can see it. A fixture page renders a Portable Text value through the public component tree, and an e2e spec reads the computed alignment from the browser. The repro test keeps the part it can prove on its own: the class the Block override emits per `textAlign`. The fixture page carries its own markup instead of a seeded entry so the suite's shared content stays untouched, which also keeps the admin visual-regression baselines unchanged. Drop the comments flagged in review and lower the changeset to the effect a reader upgrading the package can observe.
github-actions
Bot
added
review/needs-rereview
|
FYI the bot doesn't read PR comments or replies to its reviews it only reads PR body and possibly other reviews (alongside commit messages). |
There was a problem hiding this comment.
Re-review of PR #2410.
Approach judgment: This is the right, minimal fix. A prior change taught the Portable Text Block override to emit has-text-align-* classes, but it never shipped the corresponding CSS, so editor alignment was lost on public pages. Adding scoped <style> rules in Block.astro matches how sibling components in packages/core/src/components/ ship their own component styles, and keeping left class-less preserves byte-for-byte rendering for existing content.
What I checked: the diff, Block.astro, the textAlignClassName helper, sibling component style patterns, the new repro render test, the new e2e test and its fixtures, and the changeset. No SQL, auth, admin-UI, or logged-out-route query changes are involved.
Status of prior findings: All three issues from the previous review are addressed in this revision:
Block.astrohas no new JSX comment justifying the scoping/leftchoice — only the existing pre-PR file-level docstring remains, which I’m not re-flagging because it predates this change.- The new repro test no longer contains the JSDoc block, Astro-internals narrative, or
#2285reference — it is now a straightforward container render test. - The test no longer asserts the
data-astro-cid-*implementation proxy. Instead, the e2e spec asserts the actual observable behavior (computedtext-align), and the unit/repro test asserts the documented class-name contract betweentextAlignand the Block override output.
Headline conclusion: The fix is clean, focused, and consistent with EmDash conventions. No further changes needed.
github-actions
Bot
added
review/approved
3 participants
What does this PR do?
Text that you center or right-align in the editor shows with the default alignment on every public page.
The cause: #1396 made the Portable Text
Blockoverride emit WordPress-style classes likehas-text-align-center, but no stylesheet ever shipped rules for them. (rg has-text-alignacross the repo finds the class-name helper, comments, and tests — no stylesheet.)The fix adds the three missing rules as a scoped
<style>inpackages/core/src/components/Block.astro. The other public components in that directory (Code.astro,Table.astro,Columns.astro, …) follow the same pattern. Left-aligned text keeps no class and no rule, so existing content renders byte-for-byte unchanged (the #1201 convention).The editor itself was never broken: the TipTap text-align extension styles the editing view inline. Only public rendering was affected.
Tests.
tests/repro/portable-text-text-align.render.test.tsrendersPortableTextthrough the container API and pins the class that theBlockoverride emits for eachtextAlignvalue. A fourth case pins that default-aligned blocks get no alignment class.e2e/tests/portable-text-alignment.spec.tsloads a fixture page and reads the computedtext-alignin the browser. If you restoreBlock.astroto its state onmain, the e2e spec fails 3 of 4 cases.Verified end-to-end with a build of
templates/blog— the client CSS asset now contains:Closes #2285
Details
emdash/ui. That touches the Comments/CommentForm CSS bleeds into all pages via emdash/ui barrel import #2039 concern about barrel CSS, but at this size a new import path did not seem worth it.has-text-align-*markup elsewhere on a page is untouched. Within the component, Astro's scoping raises specificity to.has-text-align-center[data-astro-cid-…], which outranks a plain.has-text-align-centerrule a site may have added while the classes shipped unstyled. Such a rule almost certainly sets the same alignment. A site that needs a different value can raise the specificity of its own rule.pnpm --filter emdash exec vitest run --config vitest.repro.config.ts(26/26);pnpm exec playwright test portable-text-alignmenton both the Node and the Cloudflare target (4/4);pnpm typecheckclean;pnpm lint:json0 diagnostics against a clean baseline; formatted withpnpm format.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. (n/a: no admin strings)AI-generated code disclosure
Screenshots / test output
Full repro suite: 5 files, 26 tests passed.