feat(platform): ship a usable editor stylesheet - #528
henrique221 wants to merge 10 commits into
Conversation
…#516) The package shipped no importable CSS: usj-nodes.css, editor/editor.css and nodes-menu.css were in the tarball only because "files" includes "src", but the exports map exposed just "." and "./package.json", so consumers could not reach them. dist/index.css contains only the comments-modal styles (they land there because marginal/comments/*.tsx import them). Hence the README telling everyone to copy the CSS and the 33 icon assets out of the repo by hand. A second, CSS-only Rollup input pulls the three stylesheets through the same build and emits dist/styles.css (51.3 kB, 9.0 kB gzip) with the 15 toolbar icons inlined as data URIs, so nothing needs copying. Consumers now write: import "@eten-tech-foundation/platform-editor/styles.css"; Nothing reachable through "." moves: dist/index.css, index.js, index.js.map and index.d.ts are byte-identical to a pre-change build, verified with cmp. That is deliberate — Platform.Bible consumes only "." and styles from its own vendored copies, which are untouched here. Notes for the next reader: - cssCodeSplit must be on: Vite defaults it off under build.lib, which would merge every entry's CSS into one asset and change dist/index.css. - lib.entry stays a string even though rollupOptions.input now drives entry resolution; vite-plugin-dts reads lib.entry, and an object there makes api-extractor fail on the export-less CSS entry. - rollupOptions.input paths are absolute: Vite passes them to Rollup verbatim, so relative ids would resolve against the cwd. - src/styles.{css,ts} are build inputs and are excluded from the tarball; the barrel's @import escapes the package root.
Review or Edit in CodeSandboxOpen the branch in Web Editor • VS Code • Insiders |
Reworks the styling entry per review on eten-tech-foundation#516. usj-nodes.css is no longer bundled: Platform generates it per project so it can respond to project stylesheets, custom.sty markers and view modes, and a fixed copy in the package would be wrong for most consumers. The README documents copying it until that generation lands. editor.css is split along the boundaries the file already labelled: the built-in toolbar (hasExternalUI: false), the ContextMenuPlugin, and the debug-only TreeView. styles.css now carries only what every editor needs; the toolbar and context menu ship as their own entries, so a consumer supplying its own UI does not pay for styles it would override anyway. The split is rule-for-rule lossless (88 selectors before and after) and the repo's own platform demo imports the new files, so nx dev platform is unchanged. Claude-Session: https://claude.ai/code/session_01R6Xec2CgkML6mYyHmqiGK5
|
Reworked to match your review on #516.
The one judgement call is in the PR description: I kept the toolbar and context menu shipping as their own entries rather than dropping them, since The split is rule-for-rule lossless (88 selectors before and after) and the repo's platform demo imports the new files, so |
irahopkinson
left a comment
There was a problem hiding this comment.
Looks good to me. I'll drop an AI review next.
@irahopkinson reviewed 15 files and all commit messages, and made 1 comment.
Reviewable status:complete! all files reviewed, all discussions resolved.
irahopkinson
left a comment
There was a problem hiding this comment.
Thanks for this — the packaging works, and I've verified the parts that are hard to eyeball. Building the branch: nx build, lint, typecheck and prettier --check are green; npm pack --dry-run gives the intended 59-file set (root styles.css and assets/images/icons/LICENSE.md in, src/{styles,toolbar,context-menu}.{ts,css} out — the files negations behave); all four exports subpaths resolve to files the build actually emits.
Your claims check out, with one nit:
- Split is lossless — confirmed independently: 85 rule blocks before and after, zero missing, zero added. The only reordering is the
verse-selected/verse-delete-armedblock moving from position 81 to 21, and no selector appears in two of the split files, so the cascade is unaffected. Thank you for keeping the post-560 verse-delete-armed rules ineditor.css— that was the bit I flagged. - Icons inline — 15/15
url()in builttoolbar.cssaredata:URIs, zero externalurl()in any built stylesheet. For the record this works because Vite resolves root-absolute CSS urls againstconfig.root=packages/platform, and every SVG is 266–956 B, under the 4096 B inline limit. Worth a comment intoolbar.css, because it looks like it shouldn't work. dist/index.cssunchanged — I rebuilt with main'svite.config.mtsand compared: 11869 bytes,cmpidentical. ThecssCodeSplitreasoning holds, anddist/index.jshas no injected CSS import.- "88 selectors before, 88 after" — this figure doesn't reproduce. I count 85 rule blocks, or 107 comma-separated selectors, both conserved exactly. The conclusion is right and I've verified it, but the number matches neither metric, so it can't serve as the audit trail it's offered as.
I have not verified "API report unchanged" — I confirmed no new .d.ts is emitted, but didn't run nx extract-api.
Inline comments below. Most are small. Four are decisions rather than defects and I've marked them as questions — the nodes-menu.css one (on src/styles.css) is the one I'd most like your view on.
Two more that don't anchor to a changed line:
src/editor/editor.css:1 — orphaned stylelint directive. /* stylelint-disable selector-no-qualifying-type */ stayed at the top of editor.css, but every qualifying-type selector moved out: button.toolbar-item, i.undo, .icon.pi, button.item.dropdown-item-active → toolbar.css; .typeahead-popover li.selected → context-menu.css. Dead where it sits, missing where it's needed. Nothing breaks today (we have no stylelint config), but please move it to the two files that now need it.
Adding a stylesheet takes six edits. demos/platform/src/styles.css, src/<n>.css, src/<n>.ts, vite.config.mts input, exports, files. Tolerable at four entries, not at eight. Not something to fix in this PR, but if the shims go (see the src/toolbar.css comment) it drops to four.
Leaving this as a comment rather than a change request — most of it is prose and packaging polish, and the substantive items are questions for you.
package.json "files" conflicted: main added the test-helpers negation while this branch added the CSS entry negations and the two published extras. Kept both. Claude-Session: https://claude.ai/code/session_01ELKYEA1GwdCHakqZUG9r4o
Review on eten-tech-foundation#528. The marker menu rode into styles.css on the README's copy list, not on any analysis of when it is needed. Its only mount is `UsjNodesMenuPlugin` (Editor.tsx:927), gated on `scrRef && !hasExternalUI` — the toolbar's condition plus a `scrRef` requirement, so it is strictly more optional than the toolbar and was the one optional thing left in core. It ships as `platform-editor/nodes-menu.css` instead. Built styles.css drops 3.63 kB -> 2.46 kB, nodes-menu.css is 1.17 kB, and the 35 rule blocks are conserved exactly across the two. The import uses the `shared` workspace alias like the TS in this package does. It could not before: shared exported only "." and "./package.json", so `shared/src/styles/nodes-menu.css` failed with `Missing "./src/styles/nodes-menu.css" specifier in "shared" package`, and a relative path out of the project is an @nx/enforce-module-boundaries error. shared is a private lib, so exporting "./styles/*" widens nothing public. Also from the same review: - `./comments.css` is gone. It pointed at the "." entry's auto-generated dist/index.css, so the name did not describe the target and any future CSS in the index graph would have shipped as "comments.css". Marginal is deprecated and due for removal, so a public subpath would only buy a breaking removal later. The comment styles stay on the README copy list. - src/toolbar.css and src/context-menu.css were single `@import`s of their editor/ siblings. The .ts entries import those directly now; built toolbar.css and context-menu.css are byte-identical (cmp). - The four CSS entries each emit an empty dist/*.js plus a .js.map. Eight files that were in the tarball, not in "exports", doing nothing — negated out of "files". - The root shim for exports-blind resolvers covered one subpath of four, so those consumers got core styles and an unresolvable toolbar, which is the default configuration. There is now one per published stylesheet. Refs: eten-tech-foundation#528 Claude-Session: https://claude.ai/code/session_01ELKYEA1GwdCHakqZUG9r4o
Review on eten-tech-foundation#528, all of it prose that was wrong or in the wrong place. - The data-URI sentence sat on src/styles.css, where it is false: after the split editor.css has zero `url()` and dist/styles.css contains no data URIs at all. All 15 are in toolbar.css, so the note lives there now, with why it works — Vite resolves root-absolute CSS urls against `config.root`, and every icon is 258-720 B, under the 4096 B default assetsInlineLimit. - The styles.css header said three things were excluded and named two; it now names all five, debug-tree-view.css included, so its omission reads as deliberate. - debug-tree-view.css said "TreeView, debug only", which undersells it: the two `pre::-webkit-scrollbar` rules are unscoped element selectors that would have restyled every consumer's code blocks the moment they imported styles.css. The header says so, and says why this one gets no subpath at all. - `/* stylelint-disable selector-no-qualifying-type */` stayed on editor.css after every qualifying-type selector moved out — dead where it sat, missing where it was needed. Moved to toolbar.css and context-menu.css. - The README's data-URI paragraph sat under the styles.css import and read as contradicting the copy-list note further down. Scoped to the toolbar. - The README now says the context menu is needed by everyone today, because ContextMenuPlugin renders unconditionally (Editor.tsx:971). It stays a separate entry on the bet that it will be gated on `hasExternalUI` like the toolbar. Refs: eten-tech-foundation#528 Claude-Session: https://claude.ai/code/session_01ELKYEA1GwdCHakqZUG9r4o
Review on eten-tech-foundation#528. The demo re-enumerated editor + nodes-menu rather than importing packages/platform/src/styles.css, so nothing in the repo exercised the composition we ship and the two could drift. It imports the bundle now, plus the three optional entries and the debug TreeView, which makes `nx dev platform` a regression test for the bundle. The built demo stylesheet holds the same 760 rule blocks as before, none added, none missing. The marker menu moves earlier in the cascade; its 9 selectors appear in neither of the files it now precedes, so nothing resolves differently. Refs: eten-tech-foundation#528 Claude-Session: https://claude.ai/code/session_01ELKYEA1GwdCHakqZUG9r4o
Review on eten-tech-foundation#528. The artwork is redistributed inlined as data URIs in dist/toolbar.css (15 icons, new here) and dist/index.css (4 icons, from CommentPlugin.css, already published on main). MIT wants the copyright notice and the permission notice included in all copies, and the 123-byte file was a project name and two URLs. It now carries the upstream text from twbs/icons verbatim. Refs: eten-tech-foundation#528 Claude-Session: https://claude.ai/code/session_01ELKYEA1GwdCHakqZUG9r4o
|
The two items in your review body that had no line to anchor to: Orphaned stylelint directive. Moved in c528e98. Six edits per stylesheet. This did not drop to four, and I want to flag that rather than let you find it. Dropping the On your |
irahopkinson
left a comment
There was a problem hiding this comment.
Re-verified the whole thing at 777cf8e in a clean worktree — fresh install, fresh build, npm pack --dry-run, lint, typecheck and prettier. Since the branch has merged main in the meantime I diffed only your four commits (c8d54f00..9b81a778) so the review surface stayed the 21 files you actually touched.
Everything I could re-derive holds:
| claim | result |
|---|---|
nodes-menu.css its own entry |
dist/nodes-menu.css 1168 B — your 1.17 kB exact |
built styles.css shrinks |
2488 B (you said 2.46 kB; mine reads 2.49 — main's merge touched editor.css after you measured) |
| toolbar / context-menu unchanged | 11804 B and 1336 B, identical to the byte |
| icons still inline | 15/15 data: URIs in toolbar.css; zero external url() in any built stylesheet |
| the 15 icons are 258–720 B | exact |
| empty JS chunks gone | zero in the tarball |
exports subpaths |
all four resolve to emitted files |
| lint / typecheck / prettier | green (one no-loop-func warning in verse-block.utils.ts, from main, not from you) |
Two figures differ from yours and neither is a problem: 79 packed files rather than 78, and 34 conserved rule blocks rather than 35. Both are explained in the threads — the first is the main merge, the second is a counting convention around @media.
Three answers you were waiting on, all in the threads: keep src/styles.css (it has two consumers now, unlike the shims); you were right about the licensing premise and I was wrong — the artwork already ships on main via dist/index.css, so this fixes a pre-existing gap rather than opening one; and thanks for flagging the six-edits arithmetic yourself rather than letting me find it — you are right that adding the three root shims put back what dropping the src/*.css shims took off.
One nit inline below, and that is the last thing from me. I will take a final pass in Reviewable.
irahopkinson
left a comment
There was a problem hiding this comment.
@irahopkinson+AI resolved 1 discussion.
Reviewable status: 0 of 21 files reviewed, 13 unresolved discussions (waiting on henrique221 and irahopkinson).
Closes part of #516 (the packaging item).
The problem
Consumers copy stylesheets out of this repo by hand. The README says so, and it means every consumer re-copies on every visual change and has to serve the toolbar icon SVGs from its own web root, because
editor.cssreferenced them by absolute URL.What ships
Four stylesheets, built to
dist/, one per thing you can turn off:The 15 toolbar icons are inlined as data URIs, so no asset copying is needed.
styles.css,context-menu.cssandnodes-menu.cssreference no images at all.Each one also has a root shim (
platform-editor/styles.cssresolving to<pkg>/styles.css) for resolvers that ignore theexportsmap, so those consumers reach all four rather than only the core one.Scope, after review on #516 and #528
usj-nodes.cssis not bundled. Platform generates it per project so it can respond to project stylesheets,custom.stymarkers and the active view mode, and the copy in this repo is a placeholder for a formatted view. A fixed copy shipped in the package would be wrong for most consumers, so the README keeps documenting the copy for it.editor.cssis split before bundling, along the boundaries the file already labelled:editor/editor.cssstyles.csseditor/toolbar.csshasExternalUI: falsetoolbar.csseditor/context-menu.csscontext-menu.cssshared/styles/nodes-menu.csshasExternalUI: falseandscrRefnodes-menu.csseditor/debug-tree-view.cssdebug: trueonlyThe marker menu started out inside the core bundle and moved out on Ira's review. Its only mount is
UsjNodesMenuPluginatEditor.tsx:927, gated onscrRef && !hasExternalUI, which is the toolbar's condition plus ascrRefrequirement. It was the one optional thing left in core.debug-tree-view.cssgets no entry and no subpath on purpose. It carries two unscopedpre::-webkit-scrollbarrules that would have restyled every<pre>on a consumer's page the moment they importedstyles.css, anddebugis an experimental dev-only prop. It stays on the copy list.There is no
./comments.csssubpath. It would have pointed at the.entry's auto-generateddist/index.css, so the name did not describe the target, and<Marginal />is deprecated and due for removal, so a public subpath would only buy a breaking removal later. Comment styles stay on the copy list.Verification
styles.cssandnodes-menu.cssexactly, none added, none missing.styles.cssis 2.46 kB, down from 3.63 kB;nodes-menu.cssis 1.17 kB.url()in the builttoolbar.cssaredata:URIs; zero externalurl()in any built stylesheet. That works because Vite resolves root-absolute CSS urls againstconfig.root, and every icon is 258-720 B, under the 4096 B defaultassetsInlineLimit. There is a comment intoolbar.csssaying so, because it looks like it should not work.src/styles.cssplus the optional files rather than re-enumerating them, sonx dev platformregresses the composition we publish. Its built stylesheet holds the same 760 rule blocks as before.npm pack --dry-runon a clean build is 78 files. All fourexportssubpaths resolve to files the build emits; the four empty CSS-entry JS chunks and their maps are negated out.lint,typecheck,test,buildandformat:checkgreen across all affected projects. API report unchanged: this is CSS plus packaging, no public API change.build.lib.entrystays a string on purpose:vite-plugin-dtsderives its entry list from it, and an object there makes api-extractor fail on the export-less CSS entries. There is a comment at the call site.One change outside
packages/platform:libs/sharednow exports./styles/*. The marker menu's stylesheet lives there, and the entry needs to reach it through the workspace alias the way the TS in this package does.sharedexported only.and./package.json, soshared/src/styles/nodes-menu.cssfailed to resolve, and a relative path out of the project is an@nx/enforce-module-boundarieserror.sharedisprivate: true, so nothing public widens.Compatibility with the separate entry-point work in #552 was checked in a combined worktree, together with #541. All four public stylesheet imports exist and survive consumer bundling. #552 keeps asset basenames at the dist root; this is needed when its JavaScript modules are preserved. When combining the Vite configs, keep the JavaScript entries in lib.entry and include both JavaScript and CSS entries in rollupOptions.input. The CSS contents are unchanged by the filename adjustment.
This change is
Visual evidence
These static component-shaped fixtures use the four CSS files emitted by a fresh build of head
777cf8ed. The fixture content is illustrative; the styles, optional boundaries, icon encoding and file sizes come from the exact PR build.Editor chrome and built-in toolbar
Optional context and marker menus