Repository navigation
Conversation
Contributor
Unit Tests
Mutation Tests
|
gohabereg
force-pushed
the
feat/remove-block-tunes
branch
from
October 9, 2026 23:06
cafb423 to
46e05e9
Compare
gohabereg
force-pushed
the
feat/block-settings-ui
branch
2 times, most recently
from
October 10, 2026 01:16
0eebcb2 to
63243e2
Compare
gohabereg
force-pushed
the
feat/remove-block-tunes
branch
from
October 10, 2026 01:16
46e05e9 to
7cbecaf
Compare
`@editorjs/ui` is the first package here that renders DOM, so it is also the first to need `jest-environment-jsdom` as an explicit dependency. Two module mappings make its sources loadable under Jest: `@codexteam/ui`'s side-effect theme imports become an empty module, and CSS Modules resolve to a proxy that answers each property with its own name, so an assertion on rendered markup names the class the source asked for. `@codexteam/*` joins `@editorjs/*` in `transformIgnorePatterns` because ui-kit ships as ESM. Declaration emit moves to `tsconfig.build.json`, the config that excludes `*.spec.ts`, so `dist` does not grow typings for the tests. The shared `base-coverage` action skipped a package missing from the base ref but not one present there without a test suite -- the case of any PR adding a first suite, this one included. It would have run `test:coverage` on the base ref and failed with "couldn't find a script", so its guard now also checks that the script exists. Co-Authored-By: Claude Opus 5 <[email protected]>
`BlockSettingsUI` renders the per-block settings popover and owns no items of its own: everything in the menu comes from providers registered through `api.plugins['block-settings'].register`, which are asked again on every open so items reflect current state. Contributions are concatenated by ascending `order` (ties in registration order) with a separator between providers, and a provider returning nothing contributes no separator either. Providers receive `blockId` as well as `blockIndex`, because the index is only true at the moment the menu was built -- an item's handler is expected to resolve the block's position from the id instead. Requesting settings for an index that resolves to no block invokes no provider and opens nothing. The menu is rebuilt rather than edited between opens. `removeItemByName` is the only removal ui-kit offers and a separator carries no name, so a fresh popover with the full item list is what keeps the previous block's items out of the accessibility tree. The swap happens inside the plugin's own holder element, so ui-kit's rendered DOM stays untouched. The event classes and the popover setup are ported from the BlockTunes work in PR #157. Co-authored-by: Betty Steger <[email protected]> Co-Authored-By: Claude Opus 5 <[email protected]>
Adds the settings button beside the plus button, appended before the roving tabindex is initialized so the actions container keeps exactly one tab stop. It follows the same menu-button contract as the plus button: an accessible name from the message catalogue, `aria-haspopup="menu"`, an `aria-expanded` kept in sync with the menu's own events, and focus taken on click so the popover has somewhere to return it on close. The toolbar now also tracks the hovered block's index, not just its element, and holds position while the menu is open -- the menu was built for one block, so moving off it mid-interaction would leave the two pointing at different blocks. Fixes `ToolboxOpenUIEvent` being constructed with an event name instead of a payload. The empty payload interface accepted the string, so listeners had been receiving `detail: 'ui:toolbox:open'`; the event's name comes from the class and was never affected. Ported from PR #157. Co-authored-by: Betty Steger <[email protected]> Co-Authored-By: Claude Opus 5 <[email protected]>
Move up, Move down and Delete become an ordinary plugin registered through the `block-settings` API at `order: 1000`, so the built-in entries sit last in the menu with no privileged path into it. This is the reference example of a former "block tune" written the way any third-party plugin would write it. Every handler resolves the block's current index from its id at activation. The menu can stay open while a collaborator inserts a block above the target or an undo runs, and acting on the index the menu was built with would move a different block. `isDisabled` is necessarily a build-time snapshot, so the handlers carry their own bounds check rather than trusting it -- a stale disabled state then only greys an item out. Delete asks first: the item turns into its own confirmation in place, and only the confirmation carries a handler. It deletes by id, not by position. The plugin looks `block-settings` up lazily at `core:ready` and stays inert when it is absent, which is what a headless Core gets. `destroy()` takes the provider back out of the menu. Behaviour and test cases ported from the internal tunes in PR #157, with the confirmation step they lacked. Co-authored-by: Betty Steger <[email protected]> Co-Authored-By: Claude Opus 5 <[email protected]>
Registers `BlockSettingsUI` and `DefaultBlockSettingsPlugin` so a bundle user gets the settings menu without wiring anything. A headless `Core` still opts in. Adds the e2e suite for it, which exercises the whole round trip through the real bundle in Chromium and WebKit: opening the menu for a hovered block, the button's `aria-expanded` and focus return, the disabled state at the document boundary, moving a block, and that deletion needs its confirmation and is abandoned when the menu is dismissed. Documents the provider API in docs/plugins.md -- including why a handler must resolve the block's position from `blockId` rather than reusing `blockIndex` -- and the four `ui:block-settings:*` events in docs/events.md. Note that the bundle's spec assertions read `ctor.name`, which for a plugin declaring a static `name` is its plugin id rather than the class name; the test mock declares one too, so it asserts a name the real module actually reports. Co-Authored-By: Claude Opus 5 <[email protected]>
The menu items were configured with `wrapperTag: 'button'`, carried over from PR #157. That is what ui-kit uses for its *inline* popover, where items are horizontal toolbar buttons and shrinking to fit the label is right. A vertical menu's styles assume a block-level wrapper, and a form control does not stretch to its container the way a div does, so each row came out as wide as its own label -- roughly 90 / 108 / 76 px for Move up / Move down / Delete. The hover highlight then covered a ragged part of each row instead of the whole row, and the UA button font replaced the editor's (13.3px Arial for 15px InterVariable). Leaving the wrapper at ui-kit's default fixes all three. Items are still exposed as `menuitem` and still activated from the keyboard by ui-kit itself, so nothing is lost on the accessibility side. Guarded by an e2e test, since only a real layout engine sees this. It measures the rows in a single pass in the page and after the webfont has settled -- taken one locator at a time, the rows are measured at different moments and a font still loading moves the menu's width between them, which WebKit reported as several pixels of drift. Confirmed the test fails with the old wrapper restored. Co-Authored-By: Claude Opus 5 <[email protected]>
The menu's items are divs carrying role="menuitem", which is what the ARIA menu pattern calls for and what the toolbox already does -- but it does mean every name, role and state on them is authored rather than inherited from a native control. Nothing was asserting that. Adds an axe audit of the menu in its open state, a keyboard-only walk through it (arrow keys, roving tabindex, Enter activating and closing), and an assertion that an unavailable action is exposed with aria-disabled while staying reachable by the arrow keys, as the APG asks. Co-Authored-By: Claude Opus 5 <[email protected]>
Three defects found reviewing the change, all from `BlockSettingsUI` trusting input it has no business trusting. A provider that threw took the whole menu down. Providers are third-party code and `#collectItems` ran all of them inside one `Promise.all`, so a single plugin's error rejected the lot, was discarded by the fire-and-forget call in the constructor, and left the menu failing to open for every block -- losing the entries of the plugins that did nothing wrong. Each provider is now isolated and a failure is reported the way `BlockRenderer` reports a tool that fails to render: it contributes nothing, and the stack says which one it was. A request that resolved to no block, or one every provider opted out of, returned without touching a menu that was already open. That menu had been built for a different block, so it stayed on screen presenting one block's settings as another's. Both paths now close it. Building a menu is asynchronous, so two overlapping requests raced and whichever finished last won. With an async provider the stale build could land second and render the wrong block, reporting a `blockId` nobody asked for. Each request now takes a token and abandons its result if a newer request has started. Each case is covered by a test that fails without the corresponding fix. Co-Authored-By: Claude Opus 5 <[email protected]>
The suite was nine top-level `test()` calls. Grouped into `test.describe` blocks -- opening the menu, presentation, keyboard operation, actions -- to match how `aria.spec.ts` groups its toolbox and inline toolbar cases, and the describe/it structure the unit suites already follow. No assertions changed. Co-Authored-By: Claude Opus 5 <[email protected]>
`ToolbarUI` was rendering the settings button, naming it, tracking its
`aria-expanded`, following block selection on its behalf and dispatching
`ui:block-settings:open` on click -- all of it state belonging to a plugin that
may not even be registered. The button and the menu are one widget, so the
plugin now builds both and announces them together through
`ui:block-settings:rendered`; the toolbar only places them.
The button is announced separately from the menu rather than wrapped inside it,
because it has to be a child of the actions container in its own right for the
toolbar's roving tabindex to see it. That arrives after the toolbar has
rendered, so mounting reassigns the single tab stop -- a button carrying no
`tabindex` would otherwise be a second one.
`ui:block-settings:open` stops being how the button works and becomes the way
anything else opens the menu. The toolbar keeps `opened`/`closed` for the one
thing that is still its business: holding position while a menu built for one
block is on screen.
The `opened` event drops its `blockId` payload, which nothing consumed.
BREAKING: `ui:block-settings:rendered` now carries `{ button, blockSettings }`,
and `ui:block-settings:opened` carries nothing.
Co-Authored-By: Claude Opus 5 <[email protected]>
The plus button and the settings button look the same but are rendered by different components, and `@apply --toolbar-button` resolves within a single file's PostCSS pass -- so moving the settings button into its own plugin had meant repeating the declarations. They now share one `.toolbar-button` class in `controls.module.pcss`, which is what the two genuinely have in common. Drops the property set's `--active` modifier along the way. Nothing ever added that class, and the `bounceIn` animation it referenced is not defined anywhere in the package. Verified in the browser as well as the suites: both buttons render identically at 26x26 from the one class, and exactly one of them holds the tab stop. Co-Authored-By: Claude Opus 5 <[email protected]>
Eleven comments added by this change ran to three or four. Same content, tightened: the plugin and provider docs, the rebuild-per-open and item-wrapper notes, the toolbar's mounting contract, the shared button stylesheet, the CSS Modules mock and the row-width regression test. Co-Authored-By: Claude Opus 5 <[email protected]>
…sting Two packages here depend on another workspace for its *types* only, and neither the build order nor the CI jobs accounted for it. `@editorjs/default-block-settings-plugin` takes `@editorjs/ui` as a devDependency and imports from it with `import type`, so that `api.plugins['block-settings']` typechecks without a runtime coupling. The build scripts order by `--topological`, which sees dependencies but not devDependencies, and run in parallel -- so the plugin was free to build before `@editorjs/ui` had emitted anything. They now order by `--topological-dev`. `@editorjs/ui` had no way to build its workspace dependencies at all: its build is `vite build`, with none of the project references that make the tsc-built packages pull `@editorjs/sdk` along, and its `build:declaration` was empty. The new `ui.yml` workflow is what exposed this -- its lint and test jobs ran against an unbuilt `@editorjs/sdk`. It gets the `build:dependencies` script `@editorjs/editorjs` and the playground already use. Reproduced each failure locally by clearing every `dist` *and* `tsbuildinfo` -- with the latter left behind, `tsc --build` believes its output is current and emits nothing, which looks like a different bug entirely. From genuinely cold: build, lint and test all clean, 1111 tests. Co-Authored-By: Claude Opus 5 <[email protected]>
Three findings from reviewing this branch, plus the housekeeping around them. The button did nothing at all for a keyboard-only user. `BlockSelectedUIEvent` was dispatched on `mouseenter` and nowhere else, so with no pointer the plugin's target index stayed `-1` and activating a control that is reachable, named and `aria-haspopup`-annotated silently opened nothing. `BlocksUI` now also reports the block the caret moves into. Reading the editor's own caret state instead would not work: it is cleared the moment focus leaves the editable, which is exactly what reaching for a toolbar does. The button did not toggle. ui-kit closes the popover from a listener that runs before the button's own handler, so the handler saw a closed menu and re-opened it, and the menu could not be dismissed from the control that advertises it. The open state is now read on `mousedown` — the last moment the answer is still true — and tracked by the plugin rather than read back off the popover, whose shown state does not survive that click. Reading one block's tool name went through `api.document.data`, which serializes every block, every text node and every inline fragment tree, on every open of the menu. `BlocksAPI.getToolByIndex` replaces it, and the default plugin's two `document.data.blocks.length` reads become `getBlocksCount()`. Alongside: the UI package's `MenuConfig` becomes `BlockSettingsMenuConfig`, since `@editorjs/sdk` already exports a different type by that name and the two are not interchangeable; `BlockSettingsUI`, `DefaultBlockSettingsPlugin` and `ToolbarUI` drop their event bus listeners on `destroy`, the bus having outlived them; and `build:declaration` emits declarations again rather than only building dependencies. Co-Authored-By: Claude Opus 5 <[email protected]>
Co-Authored-By: Claude Opus 5 <[email protected]>
#collectItems awaits Promise.all over the providers, which has no floor of its own: a provider that never settles meant the menu never opened, and never opened again either, because every later attempt waited on the same provider. Each one now races a timeout, and the expiry lands in the same skip-and-log path as a provider that threw. A close arriving while the menu is still being built was also ignored, because #isOpen is only set once the providers have returned -- so the menu appeared a moment after the caller asked for it to be gone. #close() abandons the request in flight before looking at the open state. destroy() had the same hole: dropping the bus listeners stops the next request, not the one already waiting on a holder that is about to be removed. Co-Authored-By: Claude Opus 5 <[email protected]>
gohabereg
force-pushed
the
feat/remove-block-tunes
branch
from
October 13, 2026 16:09
7cbecaf to
02eff13
Compare
gohabereg
force-pushed
the
feat/block-settings-ui
branch
from
October 13, 2026 16:09
63243e2 to
c115815
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Implements the
block-settings-uichange from #189, the last of the three that replace the Block Tune entity. Stacked on #193 — review that first; this diff is the UI and the plugin.A "tune" is now just a plugin that puts an item in a menu.
BlockSettingsUIrenders the menu and owns no items of its own; everything in it comes from providers registered throughapi.plugins['block-settings'], the built-in Move up / Move down / Delete included. Nothing in the editor has a privileged path into that menu.What's in it
BlockSettingsUI(@editorjs/ui) — the menu, plusregister(provider, options?)returning an unregister function. Providers are asked again on every open, so item state is derived rather than cached; contributions are concatenated by ascendingorderwith a separator between providers, and one that returns nothing adds no separator either.@editorjs/default-block-settings-plugin— the built-in entries, registered atorder: 1000so they sit last. 100% mutation score.packages/uigets its first test suite — Jest with jsdom, CSS Modules mapped to their own class names.Three things worth a reviewer's attention
The plugin owns its button, not the toolbar.
ToolbarUIwas naming it, tracking itsaria-expandedand following block selection on its behalf — all state belonging to a plugin that may not be registered. The plugin now announces the button and the menu together throughui:block-settings:rendered, and the toolbar only places them. The button is announced separately from the menu rather than nested in it, because the toolbar's roving tabindex reads direct children of its actions container: wrapped, it would silently drop out of the single-tab-stop contractmainalready asserts in e2e.Handlers resolve the block from
blockId, neverblockIndex. A menu can stay open while a collaborator inserts a block above the target, or while an undo runs.isDisabledis necessarily a build-time snapshot, so the built-in handlers carry their own bounds check rather than trusting it — a stale disabled state then only greys an item out, where a stale index would move the wrong block.Providers are isolated. This is a public extension point whose whole job is running third-party code. One provider throwing used to take the entire menu down for every block, losing the entries of plugins that did nothing wrong; each is now wrapped, and a failure is reported the way
BlockRendererreports a tool that fails to render.Naming
@editorjs/block-actions-pluginbecame@editorjs/default-block-settings-plugin. #187 ships "Block actions" as the toolbar container's accessible name, asserted in two e2e suites and carried in a requirement title; the plugin was still only spec text, so it was far the cheaper side to move. Rejectedblock-operations—collaboration-manageralready owns that word.Found and fixed while building it
wrapperTag: 'button'is right for ui-kit's inline popover, wrong for a vertical menu. Guarded by an e2e test, since only a real layout engine sees it.ToolboxUI's open event was constructed with an event name where a payload belongs; the empty payload type accepted the string, so every listener had been receivingdetail: 'ui:toolbox:open'.base-coverageaction would have failed on this very PR: it skipped a package missing from the base ref, but not one present there without a test suite — the case of any PR adding its first.Deliberately out of scope
Three pre-existing issues found while reviewing this, each carded on the project board rather than folded in: UI components mounting in registration order, the editor never activating a
@codexteam/uitheme (sovar(--accent--*)is dead), and the e2e suites not yet grouped ontotest.describe. None is introduced here and each touches files this change does not otherwise go near.Notes for review
## Purpose— can only happen after/archive.Verified:
yarn buildandyarn lintclean, 1,111 unit tests, 100 e2e across Chromium and WebKit,openspec validate block-settings-ui --type changepasses.🤖 Generated with Claude Code