feat(Canvas): Replace autofit by keeping zoom level and ensuring selected item is visible #1469
Conversation
This file contains 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
lordrip
force-pushed
the
fix/remove-autozoom
branch
from
26f3f20 to
4ab3e40
Compare
lordrip
commented
Sep 23, 2024
Comment on lines
192
to
204
| useEffect(() => { | ||
| const timeoutId = setTimeout( | ||
| action(() => { | ||
| controller.getGraph().fit(80); | ||
| }), | ||
| 500, | ||
| ); | ||
|
|
||
| return () => { | ||
| clearTimeout(timeoutId); | ||
| }; | ||
| }, [controller, selectedIds]); | ||
|
|
There was a problem hiding this comment.
Removing the zoom-out functionality to fit the entire flow.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1469 +/- ##
=======================================
Coverage ? 68.98%
Complexity ? 26
=======================================
Files ? 272
Lines ? 7752
Branches ? 1496
=======================================
Hits ? 5348
Misses ? 2401
Partials ? 3 ☔ View full report in Codecov by Sentry. |
lordrip
force-pushed
the
fix/remove-autozoom
branch
from
4ab3e40 to
78d1c32
Compare
lordrip
force-pushed
the
fix/remove-autozoom
branch
from
78d1c32 to
5bedff2
Compare
dlabaj
reviewed
Sep 23, 2024
apupier
requested changes
Sep 24, 2024
|
note: for a more positive vision (and I think more accurate), I would replace |
lordrip
changed the title
lordrip
commented
Sep 25, 2024
| const lastUpdate = useMemo(() => Date.now(), [props.entities]); | ||
|
|
||
| return ( | ||
| <div className={`canvas-surface ${props.className ?? ''}`}> | ||
| <ErrorBoundary key={lastUpdate} fallback={props.fallback ?? <CanvasFallback />}> | ||
| <VisualizationProvider controller={controller}> |
There was a problem hiding this comment.
Moving the VisualizationProvider here, to avoid recreating it whenever the entities array change.
lordrip
commented
Sep 25, 2024
Comment on lines
90
to
92
| requestAnimationFrame(() => { | ||
| controller.fromModel(model, true); | ||
| }); |
There was a problem hiding this comment.
Added the requestAnimationFrame
cc @jeff-phillips-18
apupier
requested changes
Sep 25, 2024
Currently, when selecting a node in the canvas, there is an existing fuctionality to center the view. This helps to keep the flow visible at all times, especially in cases when the side panel might overlap the flow. With complex flows, this behavior turns to be problematic, as the user might be zooming over a section of the flow, then selecting a node, effectively moving the viewport somewhere else. The fix is to remove the auto-center functionality so the zoom is ketp between node editing. fix: https://issues.redhat.com/browse/KTO-461
Currently, the visible-flow provider is initialized to an empty object `{}` and then updated through an `useEffect()`, causing that for the first render, it gets rendered twice.
The fix is to use the `initializer` paramter from the `useReducer` hook.
Currently, when using chromium-based browsers, navigating to from a page
to the Design page, makes the nodes render distorted.
There are a few elements contributing to this behavior, first of all,
the `visible-flow` provider was initialized to `{}`, causing to the `Canvas`
component to be rendered twice, and after that, it seems like the
`useEffect` callback that called `controller.fromModal` was batched
after the `VisualizationSurface` was initialized, missing the update.
The fix is to replace the `useEffect` callback with `useLayoutEffect` to
ensure the nodes and edges are available before the `VisualizationSurface`
component.
fix: KaotoIO#1352
In order to preserve the nodes visible when opening the configuration panel, this commit schedules a `panIntoView` command to the selected node, preserving the zoom level while aiming for the less movement possible to make the node visible. relates: https://issues.redhat.com/browse/KTO-461
lordrip
force-pushed
the
fix/remove-autozoom
branch
2 times, most recently
from
a17e379 to
7a6d574
Compare
When rendering the graph, we can see some flickering in a few situations: 1. The app is loaded for the first time or refreshed 2. Switching between no routes in the canvas and a adding a route 3. Navigating to a pages and go backto the design page 4. Disabling and enabling steps in the canvas 5. Switching the layout between horizontal and vertical The fix for those situations is: 1. Adding an `initialized` state to
lordrip
force-pushed
the
fix/remove-autozoom
branch
from
7a6d574 to
ed422f6
Compare
|
apupier
approved these changes
Sep 26, 2024
|
Thanks @apupier for the reviews 👍 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Context
Currently, when selecting a node in the canvas, there is an existing functionality to center the view.
This helps to keep the flow visible at all times, especially in cases when the side panel might overlap the flow.
With complex flows, this behavior turns out to be problematic, as the user might be zooming over a section of the flow, and then selecting a node, effectively moving the viewport somewhere else.
In addition to that, when using chromium-based browsers, navigating from a page to the Design page, makes the nodes render distorted.
There are a few elements contributing to this behavior, first of all, the
visible-flowprovider was initialized to{}, causing theCanvascomponent to be rendered twice, and after that, it seems like theuseEffectcallback that calledcontroller.fromModalwas batched after theVisualizationSurfacewas initialized, missing the update.Changes
The fix for the former is to remove the auto-center functionality so the zoom is kept between node editing, and for the latter, the fix is to replace the
useEffectcallback withuseLayoutEffectto ensure the nodes and edges are available before theVisualizationSurfacecomponent.Notes
Both fixes are complementary to each other, so they need to be merged together.
fix: #1352
fix: https://issues.redhat.com/browse/KTO-461