-
Notifications
You must be signed in to change notification settings - Fork 15.8k
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
Default menu items for 'Edit' and 'Window' #2814 #8880
Conversation
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.
Left a few minor comments, thanks for implementing this.
docs/api/menu-item.md
Outdated
@@ -64,6 +64,9 @@ The `role` property can have following values: | |||
* `zoomin` - Zoom in the focused page by 10% | |||
* `zoomout` - Zoom out the focused page by 10% | |||
|
|||
* `menuEdit` - Whole default "Edit" menu (Undo,Copy, etc.) |
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.
Should be a space after Undo,
docs/api/menu-item.md
Outdated
@@ -64,6 +64,9 @@ The `role` property can have following values: | |||
* `zoomin` - Zoom in the focused page by 10% | |||
* `zoomout` - Zoom out the focused page by 10% | |||
|
|||
* `menuEdit` - Whole default "Edit" menu (Undo,Copy, etc.) | |||
* `menuWindow` - Whole default "Window" menu (Minimize, Close, etc.) |
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.
What do you think about calling these windowMenu
and editMenu
instead? That reads a little clearer to me.
@@ -11,7 +11,7 @@ const MenuItem = function (options) { | |||
for (let key in options) { | |||
if (!(key in this)) this[key] = options[key] | |||
} | |||
|
|||
this.submenu = this.submenu || roles.getDefaultSubmenu(this.role) |
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 spec for these new cases in the https://github.com/electron/electron/blob/master/spec/api-menu-spec.js#L424 block?
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.
Added - please check next commits.
lib/browser/api/menu-item-roles.js
Outdated
} : {}, | ||
|
||
process.platform === 'darwin' ? { | ||
label: 'Bring All to Front', |
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.
Is this label needed here? Shouldn't it use the one defined by the front
role if unspecified?
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.
Yes, the label is unnecessary - fixed.
lib/browser/api/menu-item-roles.js
Outdated
// remove empty objects from within the submenu | ||
if (Array.isArray(submenu)) { | ||
submenu = submenu.filter(function (n) { | ||
return n.constructor !== Object || Object.keys(n).length > 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.
What about if instead of empty objects you just had null
in the data structure, then this check becomes much simpler as return n == null
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.
fixed
Thanks for this, pushed one minor commit that tweaked the formatting a bit 🚀 🎸 |
New roles menuEdit and menuWindow were added to layout a kind of default Edit and Window submenu to the top menu bar. If there is a submenu in the item, default layout do not apply. Layout slightly differs for MacOS and Windows.
Closes #2814