Skip to content
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

NodeGraph : Avoid confusing box navigation #2347

Merged
merged 2 commits into from
Dec 1, 2017

Conversation

johnhaddon
Copy link
Member

Previously we would happily dive down into any box, even if it was totally unrelated to the current root, or several nesting levels deep. We now only jump if the new root is a descendant of the current root, and if it is nested several levels deep we step towards it one level at a time.

Fixes #2343

@dbr
Copy link

dbr commented Nov 14, 2017

Ah, yes, this is a much neater approach than alternating the global selection.

I think the only place where #2344 wins is there is a visual indication of which box will be opened (whereas with this approach the highlighted node will be buried in the nested group somewhere). Perhaps when a node is selected, all the parent boxes would be "partially highlighted" (something like a stripped highlight or only-node-border highlight or something like that)

@johnhaddon
Copy link
Member Author

I think the only place where #2344 wins is there is a visual indication of which box will be opened (whereas with this approach the highlighted node will be buried in the nested group somewhere)

Good point. I did originally start with an even simpler approach - only diving into the selected box if it was parented to the current root. This meant there was always a visual indication of where you were going, at the expense of an Up/Up/Down/Down sequence not taking you back into the most nested box. Instead, after popping up two levels, you would need to reselect the outer box to go back down. If that doesn't seem like too much of a drawback, I'd happily update the PR with the simpler version.

Perhaps when a node is selected, all the parent boxes would be "partially highlighted" (something like a stripped highlight or only-node-border highlight or something like that)

That doesn't seem like an unreasonable suggestion, but it doesn't seem high enough priority that we'd get to it any time soon...

Previously we would happily dive down any box, even if it was totally unrelated to the current root, or several nesting levels deep. We now only jump if the new root is a descendant of the current root, and if it is nested several levels deep we step towards it one level at a time.

Fixes GafferHQ#2343
As suggested on GafferHQ#2347, although the previous approach was an improvement, it was potentially confusing to navigate when there was no visual indication of where you would go, due to the selected node being at a deeper nesting. Now we only navigate into a box if it is currently visible.
@johnhaddon
Copy link
Member Author

I did originally start with an even simpler approach - only diving into the selected box if it was parented to the current root. This meant there was always a visual indication of where you were going, at the expense of an Up/Up/Down/Down sequence not taking you back into the most nested box. Instead, after popping up two levels, you would need to reselect the outer box to go back down. If that doesn't seem like too much of a drawback, I'd happily update the PR with the simpler version.

I've added a second commit which goes back to this original approach. The first commit is still there as a record of the more complex approach should we decide that is preferable after all. I'll merge this once the tests pass.

@johnhaddon johnhaddon merged commit e155457 into GafferHQ:master Dec 1, 2017
@johnhaddon johnhaddon deleted the boxNavigation branch January 4, 2018 18:03
andrewkaufman pushed a commit to andrewkaufman/gaffer that referenced this pull request Feb 16, 2018
As suggested on GafferHQ#2347, although the previous approach was an improvement, it was potentially confusing to navigate when there was no visual indication of where you would go, due to the selected node being at a deeper nesting. Now we only navigate into a box if it is currently visible.
andrewkaufman pushed a commit to andrewkaufman/gaffer that referenced this pull request Feb 21, 2018
As suggested on GafferHQ#2347, although the previous approach was an improvement, it was potentially confusing to navigate when there was no visual indication of where you would go, due to the selected node being at a deeper nesting. Now we only navigate into a box if it is currently visible.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants