Skip to content
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

Add sheet-begin and sheet-end events to macOS BrowserWindow #9108

Merged
merged 2 commits into from
Apr 20, 2017

Conversation

yuya-oc
Copy link
Contributor

@yuya-oc yuya-oc commented Apr 4, 2017

These would be helpful when a dialog is opened without using BrowserWindow's modal option or dialog module.

For example, when a dialog is opened by <input type="file"> of a webview, the webview loses focus after the dialog is closed. This new event can tell that the dialog has been closed in order to get focus again.

Copy link
Contributor

@kevinsawicki kevinsawicki left a 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 adding this 👍

@@ -1191,6 +1191,41 @@ describe('BrowserWindow module', function () {
})
})

describe('begin-sheet event', function (done) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This shouldn't take a done callback, the it function should.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't notice the mistake, thanks!

})
})

describe('end-sheet event', function (done) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This shouldn't take a done callback, the it function should

return
}
it('emits when window opens a sheet', function () {
let sheet
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be a variable declared at the root describe level and this window should be cleaned up in an afterEach to make sure it does not stick around.

afterEach(function () {
  return closeWindow(sheet).then(function () { sheet = null })
})

@@ -498,6 +498,14 @@ Returns:

Emitted on 3-finger swipe. Possible directions are `up`, `right`, `down`, `left`.

#### Event: 'begin-sheet' _macOS_
Copy link
Contributor

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 sheet-begin and sheet-end instead?

Then they would follow the naming pattern of the scroll-touch-* events.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree. I was not able to decide because there is also the pattern of *-full-screen events.

@kevinsawicki kevinsawicki self-assigned this Apr 11, 2017
@yuya-oc yuya-oc changed the title Add begin-sheet and end-sheet events to macOS BrowserWindow Add sheet-begin and sheet-end events to macOS BrowserWindow Apr 15, 2017
@yuya-oc
Copy link
Contributor Author

yuya-oc commented Apr 16, 2017

Updated. Now sheet-begin and sheet-end events would be emitted.

@kevinsawicki kevinsawicki merged commit dc8b439 into electron:master Apr 20, 2017
@kevinsawicki
Copy link
Contributor

Thanks for this @yuya-oc 👍 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants