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

Refactor menu.popup #11968

Merged
merged 13 commits into from Feb 21, 2018

Conversation

Projects
None yet
4 participants
@codebytere
Member

codebytere commented Feb 20, 2018

Removes support for old way of calling menu.popup by 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

codebytere added some commits Feb 20, 2018

@codebytere codebytere requested a review from electron/reviewers as a code owner Feb 20, 2018

@MarshallOfSound

I think I wrapped my head around this logic 😆 LGTM

// win.popup(callback)
} else {
callback = window
}

This comment has been minimized.

@ckerr

ckerr Feb 20, 2018

Member

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

codebytere added some commits Feb 20, 2018

@codebytere codebytere requested a review from electron/docs as a code owner Feb 20, 2018

@@ -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)`

This comment has been minimized.

@ckerr

ckerr Feb 20, 2018

Member

This refactor gets rid of so much ambiguous, confusing code. Enthusiastic 👍 on moving all the optional arguments into the options object.

[newPosition, newY, newX, newWindow] = [y, x, window, null]
}
Menu.prototype.popup = function (options) {
let {window, x, y, positionItem, callback} = options

This comment has been minimized.

@ckerr

ckerr Feb 20, 2018

Member

I think this breaks? The options argument is positioningItem but this line extracts positionItem

This comment has been minimized.

@codebytere

codebytere Feb 20, 2018

Member

ohh you're totally right

codebytere and others added some commits Feb 20, 2018

@ckerr

ckerr approved these changes Feb 20, 2018

@ckerr

This comment has been minimized.

Member

ckerr commented Feb 20, 2018

CI is failing because some of the sample code in electron-typescript-definitions is expecting two arguments.

@codebytere electron/electron-typescript-definitions#95 needs review. Once that PR is merged, this popup refactor should lint cleanly

window: event.sender.getOwnerBrowserWindow(),
x: params.x,
y: params.y,
callback: callback

This comment has been minimized.

@codebytere

codebytere Feb 21, 2018

Member

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
})

ckerr and others added some commits Feb 21, 2018

@jkleinsc jkleinsc merged commit 2a97e48 into master Feb 21, 2018

9 checks passed

WIP ready for review
Details
ci/circleci: electron-linux-arm Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-arm-test Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-arm64 Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-arm64-test Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-ia32 Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-x64 Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/jenkins/pr-head This commit looks good
Details

@jkleinsc jkleinsc deleted the refactor-menu-popup branch Feb 21, 2018

facebook-github-bot added a commit to facebook/nuclide that referenced this pull request May 18, 2018

Fix deprecated usages of electron$Menu.popup API
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

hansonw added a commit to facebook-atom/atom-ide-ui that referenced this pull request May 18, 2018

Fix deprecated usages of electron$Menu.popup API
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

tzarebczan added a commit to lbryio/lbry-desktop that referenced this pull request Aug 10, 2018

fix: context menu
Breaking change in Electron 2.0
electron/electron#11968

@tzarebczan tzarebczan referenced this pull request Aug 10, 2018

Merged

fix: context menu #1865

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment