-
Notifications
You must be signed in to change notification settings - Fork 636
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-3486] Extensions menu #11541
[DYN-3486] Extensions menu #11541
Conversation
update master
update master
This reverts commit c78dfe9.
Update master
Update master
Master update from public repo
Update master
master update
Update master
Update master
Update master
Update master
Update branch
update master
Update branch
Update master
update master
Update Branch
Update master
Update master branch 4-3
Menu ux update
update master branch 12/3
Test Result (3 failures / +3) |
@QilongTang, I just updated the pr with a fix for these |
@@ -23,6 +23,7 @@ internal static string ToDisplayString(this MenuBarType type) | |||
case MenuBarType.View: return Properties.Resources.DynamoViewViewMenu; | |||
case MenuBarType.Help: return Properties.Resources.DynamoViewHelpMenu; | |||
case MenuBarType.Packages: return Properties.Resources.DynamoViewPackageMenu; |
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.
Can you add a TODO to the cases above mentioning these will be deleted in Dynamo 3.0?
@@ -204,7 +224,8 @@ public enum MenuBarType | |||
Edit, | |||
View, | |||
Help, | |||
Packages | |||
Packages, | |||
Extensions |
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.
Can you add a TODO to the cases above mentioning these will be deleted in Dynamo 3.0?
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.
LGTM with a few comments
@QilongTang I added the TODO's as you asked and also modify my implementation of the new api so that it doesn't use the code that is planed to be removed so there will be less changes needed in the future. Let me know if you have any further comments, thanks! |
@@ -70,7 +70,7 @@ public override void Loaded(ViewLoadedParams viewLoadedParams) | |||
this.documentationBrowserMenuItem = new MenuItem { Header = Resources.MenuItemText, IsCheckable = true }; | |||
this.documentationBrowserMenuItem.Checked += MenuItemCheckHandler; | |||
this.documentationBrowserMenuItem.Unchecked += MenuItemUnCheckedHandler; | |||
this.viewLoadedParamsReference.AddMenuItem(MenuBarType.View, this.documentationBrowserMenuItem); | |||
this.viewLoadedParamsReference.AddExtensionMenuItem(this.documentationBrowserMenuItem); |
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, I think this makes most sense given none of the other MenuBarTypes will be supported for the first version of this API, if we plan to support them in the future, we can add another version of this API.
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.
LGTM
Purpose
Add a new Extensions menu which will include a menu item for each extension added in alphabetical order
Picture of how it looks with Dynamo's default extensions:
If for some reason no extensions are added then the menu will be disabled:
Declarations
Check these if you believe they are true
*.resx
filesReviewers
Aaron Tang (@QilongTang )
FYIs
Roberto Tellez (@RobertGlobant20 )