refactor(graph): move the viewport translation members to PanningMixin - #1132
Open
redfish4ktc wants to merge 2 commits into
Open
refactor(graph): move the viewport translation members to PanningMixin#1132redfish4ktc wants to merge 2 commits into
redfish4ktc wants to merge 2 commits into
Conversation
WalkthroughThe change moves scrollbar-aware state and panning methods into ChangesScrollbar-aware panning
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant Graph
participant PanningMixin
participant PanningManager
participant Container
Graph->>PanningMixin: scrollPointToVisible(x, y)
PanningMixin->>Container: adjust scroll offsets or clipping dimensions
PanningMixin->>PanningManager: delegate automatic panning when enabled
Graph->>PanningMixin: center(horizontal, vertical)
PanningMixin->>Container: center scroll positions when scrollbars are present
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
This was referenced Aug 10, 2026
redfish4ktc
force-pushed
the
refactor/move_viewport_translation_to_panning_mixin
branch
from
August 11, 2026 13:31
0244662 to
26dd9b8
Compare
tbouffard
marked this pull request as draft
August 11, 2026 13:49
tbouffard
marked this pull request as ready for review
August 12, 2026 04:20
redfish4ktc
force-pushed
the
refactor/move_viewport_translation_to_panning_mixin
branch
from
August 12, 2026 04:45
26dd9b8 to
f428118
Compare
Base automatically changed from
refactor/group_and_test_per_instance_properties
to
main
August 12, 2026 04:50
AbstractGraph still held members that belong to the panning concern and have no reason to sit in the class. They all adjust the viewport without touching the scale, which is what PanningMixin already does with scrollCellToVisible and scrollRectToVisible. Move scrollPointToVisible, center, and the ignoreScrollbars and translateToScrollPosition flags with their getters. scrollPointToVisible reads isTimerAutoScroll, isAllowAutoPanning, getPanDx, getPanDy and the PanningHandler plugin, which PanningMixin owns entirely, and PanningMixin.type.ts already referenced the method as if it lived there. center is included on purpose although FitPlugin looks like the obvious target. It never touches view.scale, it only updates view.translate and the container scroll position, and it branches on hasScrollbars exactly like scrollRectToVisible. Scale is the dividing line in this codebase, ZoomMixin and FitPlugin own it while PanningMixin owns translate and scroll offset, so center belongs here. The name is what makes FitPlugin look right: fitCenter centers as a consequence of fitting, deriving the translation from a newly computed scale, whereas center works at the current scale. The public API is unchanged, mixin members are merged into the AbstractGraph interface by declaration merging. One nuance for consumers: isIgnoreScrollbars and isTranslateToScrollPosition were arrow-function class properties, so they were bound to their instance and could be detached. They are now prototype methods, like every other mixin member, so `const f = graph.isIgnoreScrollbars; f()` no longer works and needs a call on the graph, or an explicit bind.
isIgnoreScrollbars and isTranslateToScrollPosition were arrow function properties, so they were bound to their instance and could be detached from the graph. As mixin members they are now prototype methods like every other one. This deserves the breaking changes list rather than a passing mention: TypeScript gives both forms the same type, so a detached reference still compiles and only fails at runtime.
tbouffard
force-pushed
the
refactor/move_viewport_translation_to_panning_mixin
branch
from
August 12, 2026 04:50
f428118 to
70b6717
Compare
|
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 8ff00a8a-464f-455b-8e91-9e5ca1ab323b
📒 Files selected for processing (4)
CHANGELOG.mdpackages/core/src/view/AbstractGraph.tspackages/core/src/view/mixin/PanningMixin.tspackages/core/src/view/mixin/PanningMixin.type.ts
Comment on lines
+106
to
+122
| /** | ||
| * Scrolls the graph to the given point, extending the graph container if specified. | ||
| * | ||
| * If the container has no scrollbars and {@link isAllowAutoPanning} returns `true`, the graph is panned through the | ||
| * {@link PanningHandler} plugin instead of being scrolled. | ||
| * | ||
| * @param x horizontal coordinate to make visible. | ||
| * @param y vertical coordinate to make visible. | ||
| * @param extend Optional boolean that specifies if the graph container should be extended. Default is `false`. | ||
| * @param border Optional distance in pixels to keep between the point and the container edge. Default is `20`. | ||
| */ | ||
| scrollPointToVisible: ( | ||
| x: number, | ||
| y: number, | ||
| extend?: boolean, | ||
| border?: number | ||
| ) => void; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Document the ignoreScrollbars exception.
When isIgnoreScrollbars() is true, scrollPointToVisible uses container scrolling even if the container has no scrollbars. Line 109 states unconditionally that this case delegates to PanningHandler. Qualify the fallback condition so the public API documentation matches PanningMixin.scrollPointToVisible.
Proposed documentation change
- * If the container has no scrollbars and {`@link` isAllowAutoPanning} returns `true`, the graph is panned through the
+ * If {`@link` isIgnoreScrollbars} returns `false`, the container has no scrollbars, and {`@link` isAllowAutoPanning}
+ * returns `true`, the graph is panned through the
* {`@link` PanningHandler} plugin instead of being scrolled.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| /** | |
| * Scrolls the graph to the given point, extending the graph container if specified. | |
| * | |
| * If the container has no scrollbars and {@link isAllowAutoPanning} returns `true`, the graph is panned through the | |
| * {@link PanningHandler} plugin instead of being scrolled. | |
| * | |
| * @param x horizontal coordinate to make visible. | |
| * @param y vertical coordinate to make visible. | |
| * @param extend Optional boolean that specifies if the graph container should be extended. Default is `false`. | |
| * @param border Optional distance in pixels to keep between the point and the container edge. Default is `20`. | |
| */ | |
| scrollPointToVisible: ( | |
| x: number, | |
| y: number, | |
| extend?: boolean, | |
| border?: number | |
| ) => void; | |
| /** | |
| * Scrolls the graph to the given point, extending the graph container if specified. | |
| * | |
| * If {@link isIgnoreScrollbars} returns `false`, the container has no scrollbars, and {@link isAllowAutoPanning} | |
| * returns `true`, the graph is panned through the {@link PanningHandler} plugin instead of being scrolled. | |
| * | |
| * @param x horizontal coordinate to make visible. | |
| * @param y vertical coordinate to make visible. | |
| * @param extend Optional boolean that specifies if the graph container should be extended. Default is `false`. | |
| * @param border Optional distance in pixels to keep between the point and the container edge. Default is `20`. | |
| */ | |
| scrollPointToVisible: ( | |
| x: number, | |
| y: number, | |
| extend?: boolean, | |
| border?: number | |
| ) => void; |
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.
Note
Stacked on top of #1131. The base of this PR is
refactor/group_and_test_per_instance_properties, so the diff shown here contains only this step. Merge #1131 first, or let GitHub merge the stack.Second implementation step of the plan recorded in ADR 0003 (#1130). No behavior change, and the public API is unchanged.
Problem
AbstractGraphstill holds members that belong to the panning concern. They all move the viewport without touching the scale, which is exactly whatPanningMixinalready does withscrollCellToVisibleandscrollRectToVisible.PanningMixin.type.tseven documentsscrollPointToVisibleas if the method already lived there, in thetimerAutoScrollandallowAutoPanningdescriptions.Changes
Moved into
PanningMixin:scrollPointToVisibleisTimerAutoScroll,isAllowAutoPanning,getPanDx,getPanDyand thePanningHandlerplugin, all owned byPanningMixinignoreScrollbars,isIgnoreScrollbarsEventsMixinandscrollPointToVisibletranslateToScrollPosition,isTranslateToScrollPositioncenterAbstractGraph.tsloses 156 lines.Why
centerlands here and not inFitPluginFitPluginlooks like the obvious target since it ownsfitandfitCenter. It is the wrong one, and the name is what makes it look right.centernever touchesview.scale. It only callsview.setTranslateand setscontainer.scrollLeft/scrollTop. Scale is the dividing line in this codebase:ZoomMixinandFitPluginown scale,PanningMixinowns translate and scroll offset.FitPlugin.fitcomputes a new scale andfitCenterappliesview.scaleAndTranslate.scrollRectToVisible, branching onhasScrollbars(container)and then handling two worlds: no scrollbars means adjustview.translate, scrollbars means adjust the scroll position.FitPluginnever reads or writesscrollLeft/scrollTopat all.fitCenterandcenteranswer different questions.fitCentercenters as a consequence of fitting, deriving the translation from the newly computed scale.centercenters at the current scale.One nuance for consumers
isIgnoreScrollbarsandisTranslateToScrollPositionwere arrow-function class properties, so they were bound to their instance and could be detached from the graph. They are now prototype methods, like every other mixin member.graph.isIgnoreScrollbars()is unaffected. Only detached references break, for instancesomeArray.some(graph.isIgnoreScrollbars)orconst f = graph.isIgnoreScrollbars, which now need an explicitbind.This is recorded in the changelog under Breaking Changes rather than as a passing note, because TypeScript gives both forms the same type: a detached reference still compiles and fails only at runtime.
Validation
Run locally on Node 24 (
.nvmrc):build,test-check, the full suite (505 tests, 60 suites),lint,check:circular-dependencies. All passing, and no test needed changing, which is the expected signal for a pure relocation.Summary by CodeRabbit
New Features
Breaking Changes