-
Notifications
You must be signed in to change notification settings - Fork 2.9k
NIFI-14191 Added the previous process group to the URL #9849
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
…rocess group for easier navigation
Thanks for the PR @homer-rich! I'll have a look. |
I haven't had a chance to fully review, but I wanted to clarify the desired behavior. The current behavior of leave process group will navigate up one level and restore the users previous viewport. Here we are trying to retain that functionality but also select the child Process Group was just exited. I have some concern at first glance (but I need to verify) that we're losing the centering behavior which would trigger when a user navigates to a selected component without having established a viewport to restore (like through a deep link). |
That is the correct interpretation of what I am trying to accomplish with the desired behavior. I was wondering if there was a way to respond differently if the link was deep vice using the Leave Group function. I'm new to JS/TS development, apologies in advance. |
Automated review is marking this PR as stale due to lack of updates in the past four months. This PR will be closed in 15 days if the stale label is not removed. This stale label and automated closure does not indicate a judgement of the PR, just lack of reviewer bandwidth and helps us keep the PR queue more manageable. If you would like this PR re-opened you can do so and a committer can remove the stale label. Or you can open a new PR. Try to help review other PRs to increase PR review bandwidth which in turn helps yours. |
This feels like a really useful feature to me. I tested it and I like how it works. To test @mcgilman's concern, I grabbed a deep link to a processor that was two groups down. Then I cleared my browser cache and cookies and restarted the browser. When I navigated to the deep link, the processor was selected and the viewport was centered in the PG. Leave Group twice also centered those viewports. I moved the viewport on the way back down to the original processor, and on the way back out the shifted viewports were restored properly. Even though I don't understand the intricacies of the Typescript code in this PR, it appears simple enough that it can be merged with thorough testing. So if this PR expires, I'm inclined to re-open and merge. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR @homer-rich. Apologies for losing track of this one. Please see the feedback below. The behavior that you were trying to work around with the call to restoreViewport
will need further consideration to preventing the centering behavior.
@@ -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()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Added the previous process group to the end of the URL so that it may be centered with the click of the zoom button when navigating up a process group via the LeaveProcessGroup function.
Summary
NIFI-14191
Tracking
Please complete the following tracking steps prior to pull request creation.
Issue Tracking
Pull Request Tracking
NIFI-14191
NIFI-14191
Pull Request Formatting
main
branchVerification
Tested using the hot reloading available via
npm nx serve
in the frontend folder.Build
mvn clean install -P contrib-check
Licensing
LICENSE
andNOTICE
filesDocumentation