Cut createMarkingMenu over to the event-driven engine - #233
Merged
Conversation
createMarkingMenu now builds and returns the totorobot-backed controller from src/engine/controller.ts instead of the RxJS Observable pipeline in navigation.ts/connect.ts. It stops taking notifySteps and stops returning an Observable: MarkingMenuNotification, exportNotification, and the conditional result type go with it. A caller now gets an already-active controller and listens with on(type, listener), matching off to remove a listener, and calls dispose() (or uses `using`) to tear it down. EngineConfig and the renderer gain the stroke and gesture-feedback styling options the old config carried (strokeColor, strokeWidth, lowerStroke*, gestureFeedback*), since the engine had no equivalents yet and dropping them silently would have been a visual regression. The renderer also gains the novice-mode start-point marker the legacy layout connection drew at the root of the upper stroke, which the engine renderer never reproduced. The E2E fixture's Observable-to-event shim is deleted; the fixture and multiple-controllers.spec.ts now drive the controller directly. Two Playwright specs cover the remaining coverage-bar items: disposal at the browser boundary, and commit-render-dispatch ordering observed from a real listener. The legacy RxJS engine, its dependency, and the rest of its call sites are untouched: removing them is a separate, later change. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
🦋 Changeset detectedLatest commit: cfcf5dc The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
Replace each with a comma, a colon, or a plain full stop, per the project's prose style. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
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.
Closes #184.
createMarkingMenustill built and returned a 'hot' RxJSObservable, even though the parent proposal in #153 replaced that pipeline with a state machine, a native pointer source, and a typed event controller, already built and tested behindsrc/engine/. This change wires the public entry point to that engine and removes the Observable path from it.createMarkingMenunow constructs the engine's controller and returns it already active. A caller listens withcontroller.on(type, listener)and stops the menu withcontroller.dispose()(or[Symbol.dispose](), forusing).notifySteps,MarkingMenuNotification,exportNotification, and the conditional result type they required are gone: every controller dispatches the fullstart/open/move/change/select/cancelevent set, and a consumer who only wants selections listens forselect.The engine's config and renderer had no equivalent yet for the stroke and gesture-feedback styling options the old config exposed (
strokeColor,strokeWidth,lowerStroke*,gestureFeedback*), or for the small dot the legacy layout drew at the root of the upper stroke once novice mode opened. Wiring the entry point straight through without them would have silently dropped a documented, tested part of the public config and changed how a gesture looks on screen, so both are added as part of this change.The end-to-end fixture's Observable-to-event shim, which let the specs target the final event shape while the RxJS engine still ran underneath, is deleted. The fixture and
multiple-controllers.spec.tsnow drive the real controller directly, and all ten existing gesture scenarios pass against it unchanged. Two new Playwright specs cover the coverage-bar items that could only be verified against a real browser:disposal.spec.tschecks that pointer input and pending work afterdispose()produce no events or visual mutations, rendered resources are released, and a seconddispose()is a no-op;dispatch-ordering.spec.tschecks, from a listener's own vantage point, that the DOM aopen/change/selectlistener sees is already the complete result of the input that triggered it.The legacy RxJS engine and its dependency are untouched. Removing them, along with the remaining internal call sites that still import
rxjs, is a separate change.To verify locally:
yarn build && yarn e2e:build && yarn e2e:testruns the full Playwright suite, including the two new specs, against the built package.yarn test:coveragecovers the unit level. The README's API section and the demo now describe the event-based controller instead of the Observable.Test plan
yarn typecheckyarn lintyarn format:checkyarn test:coverage(318 tests, coverage above the repo's threshold)yarn build && yarn e2e:build && yarn e2e:test(34 Playwright tests, including the two new coverage-bar specs)yarn package:lint,yarn package:types,yarn package:smoke-test🤖 Generated with Claude Code