merging menu/titlebar #55100
merging menu/titlebar #55100
Conversation
| @@ -14,7 +14,6 @@ import { IViewletService } from 'vs/workbench/services/viewlet/browser/viewlet'; | |||
| import { IStorageService, StorageScope } from 'vs/platform/storage/common/storage'; | |||
There was a problem hiding this comment.
Changes in this file make sense. You are using titlebarheight instead of headingHeight since we are mergin now these elements.
| @@ -106,15 +106,7 @@ export class MenubarPart extends Part { | |||
| private _modifierKeyStatus: IModifierKeyStatus; | |||
There was a problem hiding this comment.
Changes in this file make sense, mostly simplification and updating of css rules.
| @@ -6,7 +6,6 @@ | |||
| .monaco-workbench > .part.titlebar { | |||
There was a problem hiding this comment.
Getting rid of order makes sense, but all these css changes would be best to tryout and not review imho.
| @@ -32,7 +32,8 @@ import URI from 'vs/base/common/uri'; | |||
| import { Color } from 'vs/base/common/color'; | |||
There was a problem hiding this comment.
You own this area and I leave this changes up to you.
Overall looks ok.
| @@ -30,7 +30,6 @@ import { SidebarPart } from 'vs/workbench/browser/parts/sidebar/sidebarPart'; | |||
| import { PanelPart } from 'vs/workbench/browser/parts/panel/panelPart'; | |||
There was a problem hiding this comment.
Makes sense, we are removing the menu bar part.
|
@sbatten I looked at the changes and they look good overall. I tested this out on mac and it does not work, starting the workbench only shows three menus and clicking on the File does no actions. Is the plan to merge this in this milestone? If yes then I would expect you to test this out on all platforms and once it works let me know so I can also test it out and then we can merge it in. After that we would create some detailed test plan items and so on.
|
|
@isidorn yes, the plan is to do this, this milestone. I discovered the issue yesterday about it not working on macOS. I will fix these and test them as you suggest. I'll let you know when ready for further testing. Thanks! |
|
@isidorn should be working on all platforms now |
|
Tried it on mac and linux and this work fine now. I believe you thouroughly tested on windows. |
|
@bpasero the menu is populated by contributions, or do you mean having the menu at a top-level should be based on contributions? |
|
@sbatten I see the issue, so basically the top level entries are hardcoded because we have no contribution story for those? |
|
@bpasero right, they just exist and can be contributed to. but I believe the menubar in the render process can be responsible for aggregating and ordering to keep it in one place |
mainly refs #53244
also refs #53252 #52920 #17180 #54575 #52954 #52952