| CARVIEW |
Navigation Menu
-
Notifications
You must be signed in to change notification settings - Fork 658
[DYN-8893] Group context menu bug fix #16369
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
[DYN-8893] Group context menu bug fix #16369
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
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.
See the ticket for this pull request: https://jira.autodesk.com/browse/DYN-8893
| @@ -740,6 +741,8 @@ private void GroupContextMenu_KeyDown(object sender, KeyEventArgs e) | |||
| { | |||
| if (e.Key == Key.O && Keyboard.Modifiers == System.Windows.Input.ModifierKeys.None) | |||
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.
why do we even have this condition?
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.
to make sure the user has pressed only "O" and not something like "Ctrl+O".
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.
No, I mean why pressing O while context menu is visible should ungroup, but I guess that is unrelated to this PR.
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.
It did not look logical to me either, but that was the previous behavior and my aim was to replicate it.
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.
Where did you see this before, that you tried to replicate? in the node context menu?
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.
The group context menu.
Open Dynamo 3.5.2 (works down to al least 3.0.0), create a group, call the group context menu, pres "o" -> the group is ungrouped
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.
Pull Request Overview
This PR fixes two UI issues in the group context menu:
- Disables the "O" hotkey for ungrouping when the in-canvas search bar is focused.
- Adjusts the header grid’s column span to prevent group names from being cut off.
Comments suppressed due to low confidence (1)
src/DynamoCoreWpf/Views/Core/AnnotationView.xaml.cs:744
- Consider adding a UI/unit test to verify that the 'O' key no longer triggers ungrouping when the search bar is focused.
if (searchBar != null && searchBar.IsKeyboardFocusWithin) return;
| @@ -2449,7 +2452,7 @@ private FrameworkElementFactory CreateHeaderGrid() | |||
|
|
|||
| // Add ContentPresenter | |||
| var contentPresenterFactory = new FrameworkElementFactory(typeof(ContentPresenter)); | |||
| contentPresenterFactory.SetValue(Grid.ColumnSpanProperty, 3); | |||
| contentPresenterFactory.SetValue(Grid.ColumnSpanProperty, 4); | |||
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.
[nitpick] The hard-coded column span of 4 is a bit opaque; consider defining a named constant or adding a comment to explain why the span changed from 3 to 4.
| contentPresenterFactory.SetValue(Grid.ColumnSpanProperty, 4); | |
| // Use a named constant for column span to improve readability and maintainability | |
| contentPresenterFactory.SetValue(Grid.ColumnSpanProperty, ContentPresenterColumnSpan); |
Copilot uses AI. Check for mistakes.
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.
the current line is consistent with the rest of the code
Purpose
Small PR for DYN-8893.
Declarations
Check these if you believe they are true
*.resxfilesRelease Notes
Reviewers
@DynamoDS/eidos
@jasonstratton.
@zeusongit
FYIs
@dnenov
@achintyabhat