Refactor menu.popup #11968
Refactor menu.popup #11968
Conversation
lib/browser/api/menu.js
Outdated
| // win.popup(callback) | ||
| } else { | ||
| callback = window | ||
| } |
There was a problem hiding this comment.
Trivial change:
if (typeof window === 'function') {
callback = window
} else {
opts = window
}It's weird that some of the optional arguments are in the options object but others aren't. If we were to put them all in the options, at least we wouldn't need this crazy typechecking
| @@ -59,18 +59,18 @@ will become properties of the constructed menu items. | |||
|
|
|||
| The `menu` object has the following instance methods: | |||
|
|
|||
| #### `menu.popup([browserWindow, options, callback])` | |||
| #### `menu.popup(options)` | |||
There was a problem hiding this comment.
This refactor gets rid of so much ambiguous, confusing code. Enthusiastic 👍 on moving all the optional arguments into the options object.
lib/browser/api/menu.js
Outdated
| [newPosition, newY, newX, newWindow] = [y, x, window, null] | ||
| } | ||
| Menu.prototype.popup = function (options) { | ||
| let {window, x, y, positionItem, callback} = options |
There was a problem hiding this comment.
I think this breaks? The options argument is positioningItem but this line extracts positionItem
There was a problem hiding this comment.
ohh you're totally right
|
CI is failing because some of the sample code in @codebytere electron/typescript-definitions#95 needs review. Once that PR is merged, this popup refactor should lint cleanly |
lib/browser/api/web-contents.js
Outdated
| window: event.sender.getOwnerBrowserWindow(), | ||
| x: params.x, | ||
| y: params.y, | ||
| callback: callback |
There was a problem hiding this comment.
when the key is the same name as the value, you can just pass once:
menu.popup({
window: event.sender.getOwnerBrowserWindow(),
x: params.x,
y: params.y,
callback
})
Summary: In Electron 2.0.0 the `menu.popup` API now only accepts options: electron/electron#11968 But this works just fine in Atom 1.25-1.27 as well, so this is a safe change. Reviewed By: matthewwithanm Differential Revision: D8045697 fbshipit-source-id: 9f7a3821426717998945bcf2b373710e73fdbb7f
Summary: In Electron 2.0.0 the `menu.popup` API now only accepts options: electron/electron#11968 But this works just fine in Atom 1.25-1.27 as well, so this is a safe change. Reviewed By: matthewwithanm Differential Revision: D8045697 fbshipit-source-id: 9f7a3821426717998945bcf2b373710e73fdbb7f
Breaking change in Electron 2.0 electron/electron#11968
Removes support for old way of calling
menu.popupby changing signature from(window, x, y, positioningItem)=>(window, opts, callback).Per 2.0 Breaking Changes.
Changed tests for old signature and added a few new tests.
/cc @MarshallOfSound @ckerr