feat(nesting): column widths, draggable rows inside columns, and a collapsed state - #2248
feat(nesting): column widths, draggable rows inside columns, and a collapsed state#2248bimsonz wants to merge 5 commits into
Conversation
🦋 Changeset detectedLatest commit: da43fca 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 |
Scope checkThis PR changes 1,618 lines across 19 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. |
|
I have read the CLA Document and I hereby sign the CLA 1 out of 2 committers have signed the CLA. |
github-actions
Bot
added
query-count changed
![emdashbot[bot]](https://avatars.githubusercontent.com/in/3255304?s=60&v=4){kind=small}
There was a problem hiding this comment.
This is a well-scoped follow-on to the approved nesting-block discussion: equal columns, non-reorderable nested rows, and the lack of a collapsed state are all real blockers the prototype had, and the approach here (view-state folding, row-level drag rules, duplicated admin converters so attributes survive the admin round-trip) fits EmDash’s architecture. The tests cover the core PT↔PM seam, the admin seam, and drag-unit behavior.
I checked the changed admin components, core converters/components, and tests. There are no new server routes, no DB queries, and no auth changes, so the logged-out hot path is unaffected. I did find an i18n issue and several AGENTS.md comment-discipline violations that should be cleaned up before this leaves draft.
Headline: approach is right, code is careful, but the collapsed-container summary and several new reviewer-facing/narrative comments need fixing.
| A `<Plural>` here renders its own ICU source until then, which is worse | ||
| to look at than an unagreed plural. */} | ||
| {collapsed && ( | ||
| <span className="text-kumo-subtle/70 text-sm"> | ||
| {columnCount} {t`columns`} | ||
| {", "} | ||
| {blockCount} {t`blocks`} | ||
| </span> |
There was a problem hiding this comment.
[needs fixing] The collapsed summary concatenates {columnCount} {t\columns`}with a literal comma and{blockCount} {t`blocks`}` (lines 341-343). That hard-codes English punctuation and word order and won’t agree for plural rules in any locale. The JSX comment above it (lines 337-340) is also framed for this review (‘until catalogs are extracted’) rather than for future readers, which violates the AGENTS.md comment rules.
Use Lingui’s plural (import from @lingui/core/macro) and drop the comment block.
| A `<Plural>` here renders its own ICU source until then, which is worse | |
| to look at than an unagreed plural. */} | |
| {collapsed && ( | |
| <span className="text-kumo-subtle/70 text-sm"> | |
| {columnCount} {t`columns`} | |
| {", "} | |
| {blockCount} {t`blocks`} | |
| </span> | |
| {collapsed && ( | |
| <span className="text-kumo-subtle/70 text-sm"> | |
| {t`${columnCount} ${plural(columnCount, { one: "column", other: "columns" })}, ${blockCount} ${plural(blockCount, { one: "block", other: "blocks" })}`} | |
| </span> | |
| )} |
Run pnpm locale:extract so the source locale gets the plural forms; that removes the ICU-source rendering issue the comment describes.
| // A stale position between transactions -- treat as top level. | ||
| return false; | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * The draggable unit is a row: a child of the document, or a child of a nesting | ||
| * column, which is the same thing one level down. | ||
| * | ||
| * This is what the editor already did. With nested targeting off, TipTap targets | ||
| * top level blocks, so a list drags as one block and its items do not drag at all. | ||
| * The rule restates that and extends it into columns, which is why behaviour | ||
| * outside a container is unchanged by enabling nesting. | ||
| * | ||
| * It deliberately replaces TipTap's default rules rather than joining them, and | ||
| * that is a choice about units, not a claim that they are wrong. Their defaults | ||
| * resolve the unit *inside* a structure: for a list, `listItemFirstChild` and | ||
| * `listWrapperDeprioritize` between them exclude the paragraph and the wrapper so | ||
| * the list item wins. That is right for a plain document and wrong for a page | ||
| * built from containers, where a list is one row a page is composed of and its | ||
| * items are the row's internals. The two cannot both hold, and picking theirs | ||
| * means a list can no longer be moved as a block anywhere in the document. | ||
| * | ||
| * Nothing the defaults guard is lost. Table internals and inline content are | ||
| * never children of the document or of a column, so they are excluded here by | ||
| * construction; the tests assert that rather than assuming it. | ||
| * | ||
| * Columns themselves are never a target either: `selectable: false`, and they are | ||
| * added and removed from the container's toolbar. | ||
| */ | ||
| export const _rowsOnlyRule: DragHandleRule = { | ||
| id: "emdashRowsOnly", | ||
| evaluate: ({ node, depth, $pos }) => { | ||
| const EXCLUDE = 1000; | ||
| if (node.type.name === "nestingColumn") return EXCLUDE; | ||
| if (depth <= 1) return 0; | ||
| return $pos.node(depth - 1).type.name === "nestingColumn" ? 0 : EXCLUDE; | ||
| }, | ||
| }; |
There was a problem hiding this comment.
[needs fixing] This oversized doc block for _rowsOnlyRule is addressed to the reviewer (‘that is a choice about units, not a claim that they are wrong’, ‘If you would rather keep your unit…’, ‘Nothing the defaults guard is lost…’). Per AGENTS.md, comments should not justify decisions, narrate rejected alternatives, or address whoever is reviewing. The behavior is already asserted by the unit tests.
Replace it with a short rationale for future readers:
| // A stale position between transactions -- treat as top level. | |
| return false; | |
| } | |
| } | |
| /** | |
| * The draggable unit is a row: a child of the document, or a child of a nesting | |
| * column, which is the same thing one level down. | |
| * | |
| * This is what the editor already did. With nested targeting off, TipTap targets | |
| * top level blocks, so a list drags as one block and its items do not drag at all. | |
| * The rule restates that and extends it into columns, which is why behaviour | |
| * outside a container is unchanged by enabling nesting. | |
| * | |
| * It deliberately replaces TipTap's default rules rather than joining them, and | |
| * that is a choice about units, not a claim that they are wrong. Their defaults | |
| * resolve the unit *inside* a structure: for a list, `listItemFirstChild` and | |
| * `listWrapperDeprioritize` between them exclude the paragraph and the wrapper so | |
| * the list item wins. That is right for a plain document and wrong for a page | |
| * built from containers, where a list is one row a page is composed of and its | |
| * items are the row's internals. The two cannot both hold, and picking theirs | |
| * means a list can no longer be moved as a block anywhere in the document. | |
| * | |
| * Nothing the defaults guard is lost. Table internals and inline content are | |
| * never children of the document or of a column, so they are excluded here by | |
| * construction; the tests assert that rather than assuming it. | |
| * | |
| * Columns themselves are never a target either: `selectable: false`, and they are | |
| * added and removed from the container's toolbar. | |
| */ | |
| export const _rowsOnlyRule: DragHandleRule = { | |
| id: "emdashRowsOnly", | |
| evaluate: ({ node, depth, $pos }) => { | |
| const EXCLUDE = 1000; | |
| if (node.type.name === "nestingColumn") return EXCLUDE; | |
| if (depth <= 1) return 0; | |
| return $pos.node(depth - 1).type.name === "nestingColumn" ? 0 : EXCLUDE; | |
| }, | |
| }; | |
| /** | |
| * Drag unit: direct children of the document or of a nesting column. | |
| * Table internals and inline content are excluded by the schema. | |
| */ | |
| export const _rowsOnlyRule: DragHandleRule = { |
| /** | ||
| * Nested targeting, so a block inside a nesting column can be reordered. | ||
| * | ||
| * Edge detection is off, and follows from the same choice. It resolves ambiguity by | ||
| * pointer position, deducting by depth near a node's edge so the parent wins there. | ||
| * With rows as the unit there is no ambiguity left to resolve: a column is never a | ||
| * target, so the only candidates are the row and its container, and the container | ||
| * has an explicit grab point of its own in its header. Leaving it on only breaks | ||
| * things, because the deduction excludes a candidate outright at the depth a row | ||
| * sits inside a column, and rows are often short enough that the 12px band covers | ||
| * half of one. | ||
| * | ||
| * The handle's placement depends on this too. The gutter it sits in belongs to the | ||
| * row, so a target deeper than a row is measured from the wrong box and the handle | ||
| * lands over the content instead of beside it. | ||
| * | ||
| * Module level so the reference is stable: DragHandle's effect depends on it, and a | ||
| * fresh object each render re-registers the plugin. | ||
| */ | ||
| export const _nestedDragOptions = { | ||
| rules: [_rowsOnlyRule], | ||
| defaultRules: false, | ||
| edgeDetection: "none" as const, | ||
| }; | ||
|
|
There was a problem hiding this comment.
[needs fixing] The comment block for _nestedDragOptions repeats the PR narrative about why edge detection is off and why the handle placement follows from that choice. Like the _rowsOnlyRule block, this is reviewer-facing justification rather than code documentation.
The option object is self-describing; remove the narrative:
| /** | |
| * Nested targeting, so a block inside a nesting column can be reordered. | |
| * | |
| * Edge detection is off, and follows from the same choice. It resolves ambiguity by | |
| * pointer position, deducting by depth near a node's edge so the parent wins there. | |
| * With rows as the unit there is no ambiguity left to resolve: a column is never a | |
| * target, so the only candidates are the row and its container, and the container | |
| * has an explicit grab point of its own in its header. Leaving it on only breaks | |
| * things, because the deduction excludes a candidate outright at the depth a row | |
| * sits inside a column, and rows are often short enough that the 12px band covers | |
| * half of one. | |
| * | |
| * The handle's placement depends on this too. The gutter it sits in belongs to the | |
| * row, so a target deeper than a row is measured from the wrong box and the handle | |
| * lands over the content instead of beside it. | |
| * | |
| * Module level so the reference is stable: DragHandle's effect depends on it, and a | |
| * fresh object each render re-registers the plugin. | |
| */ | |
| export const _nestedDragOptions = { | |
| rules: [_rowsOnlyRule], | |
| defaultRules: false, | |
| edgeDetection: "none" as const, | |
| }; | |
| export const _nestedDragOptions = { | |
| rules: [_rowsOnlyRule], | |
| defaultRules: false, | |
| edgeDetection: "none" as const, | |
| }; |
| > | ||
| {/* No grip of its own -- see PluginBlockNode. The editor's drag handle covers | ||
| every block, and -start-8 puts this one in a gutter a nesting column does | ||
| not have. */} | ||
| <div className="relative group"> |
There was a problem hiding this comment.
[needs fixing] This JSX comment explains why a duplicate grip was removed. That context belongs in the commit message/PR description, not in a code comment. Delete the block.
| > | ||
| {/* No grip of its own: the editor's drag handle already offers one for every | ||
| block, and the wrapper above carries data-drag-handle, so a third one only | ||
| ever sat underneath the other two. It was positioned at -start-8, in the | ||
| editor's left gutter, which does not exist inside a nesting column -- there | ||
| the grip hung outside the column, over its neighbour or outside the | ||
| container entirely. */} |
There was a problem hiding this comment.
[needs fixing] This JSX comment justifies why the duplicate grip was removed. Per AGENTS.md, comments should not narrate rejected alternatives or address the reviewer. Delete the block.
Builds on the nesting block prototype with the two things that stopped it being usable on a real page: columns could not be given different widths, and the blocks inside a column could not be reordered. Column widths Equal columns cannot express a content-plus-sidebar page, which is the most common two-column layout there is. `repeat(N, minmax(0, 1fr))` forces 50/50 at two columns, so a sidebar comes out the same width as the main content. Adds a `widths` attribute with presets: equal, wide-first, wide-last, narrow-first, narrow-last. The weighted column takes 2fr against 1fr for the rest, which gives the familiar two-thirds/one-third split at two columns and stays sensible above that. Unrecognised values fall back to equal, and the existing collapse to a single column under 768px is unaffected. The control is disabled for a flex container, where columns size from their content and neither the editor nor the site renderer applies a ratio. Note the admin carries its own Portable Text converters, separate from the ones in core. An attribute added to core alone passes core's round-trip tests and is then dropped by every real save, which is what happened here: the preset applied on screen, saved without complaint, and came back as "Equal" on reload. Both sides now carry it, and the new test asserts a full PT to PM to PT cycle rather than a single direction. Rows inside columns A block in a column had no drag handle, so it could be typed into but not reordered. TipTap only considers top level nodes unless nested targeting is switched on, and a block in a column is three levels deep. Enabling it needs very little. A block inside a column already scores full marks under the default rules, and ties resolve to the deepest candidate, so it beats its column and its container unaided. The one thing the defaults cannot know about is that `nestingColumn` is `selectable: false` and must never be a drag target, so that is the only rule added. The defaults are left on: they are what keeps list and table internals from becoming targets, and a list item behaves identically inside a column and outside one. Edge detection drops the top edge and keeps the left. It deducts by depth within 12px of an edge, enough to exclude a candidate outright at the depth a row sits inside a column. On the left that is wanted, and it is how a blockquote is grabbed by its border. On the top it is not: rows are often 28px, so the band covers half the row, and because a column is not a drag target the promotion lands two levels up on the container rather than on the immediate parent the rule is meant to prefer. This is the one shared default the PR changes. The handle also needs somewhere to go. The editor reserves a 64px gutter down its own edge; a column has none, so a nested handle landed on the column beside it or outside the container. The gutter is padded onto each row rather than onto the column, because padding the column leaves it outside every row's box and a pointer there resolves to the column, which is not a target, so the handle jumps away as the editor reaches for it. The container keeps a grab point of its own, since hovering it resolves to the deepest row inside it, but it is the header rather than a permanently visible grip. Nothing else in the editor shows a handle when it is not hovered. Two duplicate grips are removed while here. Plugin and HTML blocks each drew their own at `-start-8`, in a gutter that does not exist inside a column, on top of the handle the editor already provides and the wrapper's own drag target. Also lets a plugin block's header wrap. Its action buttons hold their width while hidden, so in a sidebar column they left the label a few pixels and it broke one character per line.
A container is a large open box with no way to close it, so a page built from several of them cannot be scanned. Adds a disclosure in the header: collapsed, the container is one row reading its own name and what it holds, "2 columns, 9 blocks". Collapsed is view state rather than a node attribute. Folding a container away is not a change to the document, so it should not mark the page dirty or land in a revision, and it resets on reload the way a disclosure is expected to. The content is hidden rather than unmounted. ProseMirror owns that element as the node's contentDOM, and removing it would detach the container's content from the document. The layout controls fold away with it, since they describe content that is not on screen. Delete stays, so an unwanted container does not have to be opened first. The counts sit outside the translated words rather than inside a plural message. A `<Plural>` renders its own ICU source until catalogues are extracted, which looks broken in the editor and in screenshots, and extracting them here is not something a feature PR should carry. The trade is that the summary is not plural agreed for locales with richer rules, which is easy to change once the catalogue exists.
The collapsed container summary was built by concatenating counts with translated words and a literal comma, which fixes English word order and punctuation. It is now a single message with plural forms for both counts. The comments added across these changes carried decision rationale and narrative about what was tried. Cut back to the invariants a reader would otherwise get wrong: the handle gutter and its offset have to agree, the admin's grid template mirrors core's, and the node's contentDOM stays mounted while collapsed.
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. |
f48485c to
da43fca
Compare
github-actions
Bot
removed
query-count changed
|
Thanks, this was a useful review. All five fixed, plus the comments you did not flag. i18n. Taken, with Comments. Fixed the four, then went through every comment this branch adds and cut the rest to the same standard, about 120 lines. One deviation worth flagging. I kept a single line on The same file already carries that note on Also rebased onto main, which resolves the conflict in the two converters (gallery and nesting each added a case, both kept) and clears the query-count diff, which was this branch sitting behind |
@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: |
3 participants
What does this PR do?
Builds on @louisescher's nesting block prototype from #2049 with the three things that stopped it being usable once I put it in front of an editor: columns could not be given different widths, the blocks inside a column could not be reordered, and a container could not be folded away.
This is opened as a draft, and is not trying to land ahead of @louisescher. His prototype commit is the base of this branch and stays attributed to him. It is opened against
mainrather than his branch because CI and a review pass only run on a PR tomain. If he would rather these commits went onto his branch, or into a PR of his own, say so and I will move them and close this.Related: #2049
Column widths
Equal columns cannot express a content and sidebar page, which is the most common two column layout there is.
repeat(N, minmax(0, 1fr))forces 50/50 at two columns, so a sidebar comes out the same width as the main content.Adds a
widthsattribute with presets:equal,wide-first,wide-last,narrow-first,narrow-last. The weighted column takes 2fr against 1fr for the rest. Unrecognised values fall back toequal, and the existing collapse to a single column under 768px is unaffected. The control is disabled for a flex container, where columns size from their content and the site renderer ignores widths too.Worth knowing before adding any further attributes: the admin carries its own Portable Text converters in
PortableTextEditor.tsx, separate frompackages/core/src/content/converters/. An attribute added to core alone passes core's round trip tests and is then dropped by every real save. That happened here, and the added test asserts a full PT to PM to PT cycle rather than a single direction.Blocks inside a column
A block in a column had no drag handle, so it could be typed into but not reordered. TipTap only considers top level nodes unless nested targeting is on, and a block in a column is three levels deep.
The unit question is the part I am least sure about. With
nestedon, the default rules resolve the unit inside a structure:listItemFirstChildandlistWrapperDeprioritizebetween them exclude a list's paragraph and its wrapper so the list item wins. That is right for a plain document. For a page built from containers a list is one row the page is made of, so this replaces them with a rule where a row is a child of the document or a child of a column.That restates what the editor does today with
nestedoff, and extends it one level down, so behaviour outside a container does not change. It also keeps a list movable as a block, which taking the defaults does not allow anywhere in the document oncenestedis on. Nothing the defaults guard is lost, because table internals and inline content are never children of the document or of a column; the tests assert that rather than assuming it, with a table cell resolving to the table and a quote's paragraph to the quote.If you would rather keep your unit, I am happy to rework it. It is a choice about what a page is made of, not a claim that the defaults are wrong.
Two smaller pieces follow from the same decision:
The container keeps a grab point of its own, since hovering it resolves to the deepest row inside it, but it is the header rather than a permanently visible grip.
Two duplicate grips are removed while here. Plugin and HTML blocks each drew their own at
-start-8, in a gutter that does not exist inside a column, on top of the handle the editor already provides and the node wrapper's own drag target. Happy to split that out if you would rather keep this to nesting.Folding a container away
A container is a large open box with no way to close it, so a page built from several cannot be scanned. Collapsed, it is one row reading its own name and what it holds, "2 columns, 9 blocks".
Collapsed is view state rather than a node attribute, so folding a container away is not a change to the document and does not land in a revision. The content is hidden rather than unmounted, because ProseMirror owns that element as the node's contentDOM.
The counts sit outside the translated words rather than in a plural message. A
<Plural>renders its own ICU source until catalogues are extracted, which looks broken in the editor and in visual regression screenshots, and extracting catalogues is not something a feature PR should carry. The trade is that the summary is not plural agreed for richer locales, which is easy to change once the catalogue exists.Not addressed here
grid-row: 1 / span 3would do it. That is a ceiling rather than a bug and is probably worth stating in the RFC.Type of change
Checklist
pnpm typecheckpassespnpm lintpassespnpm testpasses (or targeted tests for my change)pnpm formathas been runmessages.pochanges are included.AI-generated code disclosure
Screenshots / test output
@emdash-cms/admin: 1224 passing across 100 files, 0 failing.emdash: 4976 passing, 1 failing. The failure istests/unit/astro/integration/virtual-modules.test.ts, which fails identically without this change, so it is not from it.oxlint --type-aware --deny-warningsclean,oxfmt --checkclean,tsgo --noEmitclean on both packages.Worth flagging separately:
packages/admin/tests/components/SeoPanel.test.tsxandtests/publish-button-locale.test.tsxeach failed during full suite runs, a different test each time, and pass repeatedly in isolation. Both are untouched by this change and look non-deterministic under parallel browser load.Behaviour was checked by driving the admin in a browser, not only through tests. Two columns measured 296/296 before the widths change and 394/197 after. With the gutter on the row, a pointer at x=801 beside a heading puts the handle at x=781, aligned to that heading and inside the row's own padding; before, the same pointer targeted the container.