mcgilman commented on code in PR #9849:
URL: https://github.com/apache/nifi/pull/9849#discussion_r2317166974
##########
nifi-frontend/src/main/frontend/apps/nifi/src/app/pages/flow-designer/ui/canvas/canvas.component.ts:
##########
@@ -172,7 +172,7 @@ export class Canvas implements OnInit, OnDestroy {
if (skipTransform) {
this.store.dispatch(setSkipTransform({ skipTransform:
false }));
} else {
- this.store.dispatch(centerSelectedComponents({ request: {
allowTransition } }));
+ this.store.dispatch(restoreViewport());
Review Comment:
This change is problematic. This was the concern I mentioned initially
though the example I gave was wrong. It's removes centering behavior and
replaces it with restoring the most recent viewport. An example of a sequence
where this becomes an issue is when using the Search capabilities to find
another component on the canvas in the current Process Group that is currently
out of view. Attempting to navigate to the component through the search results
won't work since the user is already centered on the current viewport.
This change shouldn't be needed though but I think I understand why it was
included. This sequence will fire twice... once for when the route changes and
once for when the store is reloaded (because we're changing Process Groups).
These both fire independently/separately. As a result, the Process Group is
centering when that wasn't the intent. I would need to consider this scenario
more carefully. Other instances of `navigateWithoutTransform` happen within the
current Process Group so the current Process Group doesn't fire and doesn't
showcase this issue.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
