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 beforeunload as first class window event #185

Merged
merged 2 commits into from
Aug 16, 2018

Conversation

bingenito
Copy link
Member

Delegate beforeunload event on concrete ContainerWindow to either underlying container implementation or window object. This pattern will facilitate creating your own non window based beforeunload in custom ContainerWindow derivations.

Follows semantics of window beforeunload by leveraging returnValue on the event argument.

container.getCurrentWindow().addListener("beforeunload", (e) => {
  e.returnValue = false;  // Cancel closing of the window
});

@bingenito bingenito self-assigned this Aug 15, 2018
@bingenito bingenito requested a review from a team August 15, 2018 13:01
@codecov
Copy link

codecov bot commented Aug 15, 2018

Codecov Report

Merging #185 into master will decrease coverage by 0.19%.
The diff coverage is 85.71%.

@@            Coverage Diff            @@
##           master     #185     +/-   ##
=========================================
- Coverage   93.54%   93.35%   -0.2%     
=========================================
  Files          15       15             
  Lines        1364     1384     +20     
  Branches      229      235      +6     
=========================================
+ Hits         1276     1292     +16     
- Misses         88       92      +4
Impacted Files Coverage Δ
src/window.ts 88.68% <ø> (ø) ⬆️
src/Electron/electron.ts 96.38% <100%> (+0.06%) ⬆️
src/events.ts 100% <100%> (ø) ⬆️
src/container.ts 96.09% <100%> (ø) ⬆️
src/OpenFin/openfin.ts 85.62% <42.85%> (-1.05%) ⬇️

this.innerWindow.addEventListener(windowEventMap[eventName] || eventName, listener);
if (eventName === "beforeunload") {
this.innerWindow.addEventListener("close-requested", (e) => {
const args = new EventArgs(this, "beforeunload", e);
Copy link
Member Author

Choose a reason for hiding this comment

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

This callback is not covered in the test and is the resulting loss of coverage


protected postProcessArgs(args: EventArgs) {
if (args && typeof args.returnValue !== "undefined") {
(<any>args.innerEvent).returnValue = args.returnValue;
Copy link
Member Author

@bingenito bingenito Aug 16, 2018

Choose a reason for hiding this comment

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

desktopJS events wrap the internal container events for usage consistency. The returnValue on the djs event args needs to be propagated back to the inner event for the specific container.

@bingenito bingenito merged commit edd6745 into morganstanley:master Aug 16, 2018
@bingenito bingenito deleted the beforeunload branch September 26, 2018 12:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants