clean up icon path confusion #41577
clean up icon path confusion #41577
Conversation
|
@jrieken ping |
|
|
||
| constructor( | ||
| action: MenuItemAction, | ||
| private action: MenuItemAction, |
| private _wantsAltCommand: boolean = false; | ||
| private _itemClassDispose: IDisposable; |
There was a problem hiding this comment.
@jrieken same as before I would say (https://github.com/Microsoft/vscode/pull/41577/files#diff-b1eab124b1401b8228c34df2fc86a5e7L303) ?
There was a problem hiding this comment.
Interesting line of argumentation... Well, now that this is created by an object with a lifecycle we could make use of that fact...
There was a problem hiding this comment.
@jrieken sorry I thought you meant something else, pushed a fix
| @IKeybindingService private _keybindingService: IKeybindingService, | ||
| @IMessageService protected _messageService: IMessageService, | ||
| @IContextMenuService private _contextMenuService: IContextMenuService | ||
| ) { | ||
| super(undefined, action, { icon: !!action.class, label: !action.class }); | ||
| super(undefined, action, { icon: !!(action.class || action.item.iconPath), label: !action.class && !action.item.iconPath }); | ||
| } |
There was a problem hiding this comment.
Not sure what that is for. Does the super-type support icon being a class-name or a file paths?
There was a problem hiding this comment.
@jrieken this is just a hint that we have an icon to show and do not expect a label
There was a problem hiding this comment.
and that hint is used somewhere?
There was a problem hiding this comment.
@jrieken here to add the icon class I guess: https://github.com/Microsoft/vscode/blob/ben/fix-35491/src/vs/base/browser/ui/actionbar/actionbar.ts#L302
| @@ -23,8 +23,7 @@ export interface ICommandAction { | |||
| id: string; | |||
| title: string | ILocalizedString; | |||
| category?: string | ILocalizedString; | |||
| iconClass?: string; | |||
| iconPath?: string; | |||
| iconPath?: string | { light: string; dark: string; }; | |||
There was a problem hiding this comment.
Looking at consumer code would it be nicer to just have { light: string; dark: string; }? Where dark defaults to light if missing
There was a problem hiding this comment.
@jrieken yeah makes sense, pushed
|
ops, forgot to press submit... |
|
lgtm |
fixes #35491