RFC/POC: Extend View system to support multi-canvas rendering - #10229
RFC/POC: Extend View system to support multi-canvas rendering#10229ibgreen wants to merge 4 commits into
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 72fa41c755
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
79b90e5 to
8ba7afe
Compare
Pessimistress
left a comment
There was a problem hiding this comment.
-
Did you test widgets at all? I did not see any changes to the widget manager on placement handling w/ multiple canvases.
-
I strongly recommend that you exclude react from the initial PR. The implementation looks very hacky and not conforming to reactive patterns. And this is adding on top of existing complex React flow with known bugs.
85cee04 to
ff8fefb
Compare
e33dd8e to
84979d1
Compare
be0136a to
53bd285
Compare
05f8f9f to
c15183d
Compare
|
@Pessimistress Addressed the requested review changes and rebased the feature on the now-merged preparation work:
Validation: core TypeScript checking, repository lint/pre-commit smoke tests, and all 90 focused browser tests pass. |
89c1f3f to
83ef05e
Compare
0954aba to
ce2ab1a
Compare
|
@Pessimistress I re-audited every review comment and updated the rebased stack:
Validation: |
Goal
Add opt-in multi-canvas presentation while preserving the existing single-canvas
DeckAPI and behavior.Compatibility and API
DeckProps.canvasretains its existing scalar type, automatic canvas creation, event-manager lifecycle, and integration behavior.DeckProps._canvasesis intentionally a separate opt-in, as agreed during review follow-up, so existing React, Mapbox, and other single-canvas integrations cannot accidentally change behavior.View.canvasIdassigns a view to one presentation canvas; omitted ids select the first configured canvas.Deck.getEventManager()already exists onmaster; this PR preserves the existing API and resolves the manager for the requested view.Changes
.deck-events-roothandling and route controllers, DOM events, and picking to their source canvas.Review stack
master.Validation
yarn— workspace dependencies updated to match currentmaster.yarn lint— passes; existing repository-wide lint warnings remain unchanged../node_modules/.bin/tsc -p modules/core/tsconfig.json --noEmit --pretty false— passes../node_modules/.bin/vitest run --project node --silent— 22 tests passed.