feat(panel): Adding a hook on close success. #9819

DerekLouie · 2016-10-12T00:15:18Z

Hello,

Here is a small PR to add an "on close success" hook for users who want to run some code after a panel successfully closes.

It also adds an optional parameter to the "close" function which allows the user to specify a string describing why the close happened (standardized strings have been added to the "escapeToClose" and "clickOutsideToClose" functions.

Please Review.

crisbeto · 2016-10-12T09:15:37Z

@ErinCoughlan @DerekLouie wouldn't this be more appropriate to pass to the onClose interceptor? That way you could use the reason for closing to determine whether to block it.

ErinCoughlan · 2016-10-12T12:49:49Z

@crisbeto Interceptors intercept the close events, so they are run before close is finished and can block the close from happening. The goal of this PR is to allow the user to run events after the panel is closed, similar to the animation hooks for onDomRemoved.

This already works for explicit close events because the user can chain off of the close call. But on pressing escape or clicking on the backdrop, we don't give the user any way to know these events happened or allow them to do work after it's closed. The most common use case of this is a dialog where when the panel is closed you need to reject/resolve the promise from open. This currently is not possible.

crisbeto · 2016-10-12T12:54:31Z

Alright, that makes sense. Regarding the promise: didn't we have a task somewhere to have it be a part of the mdPanel API (returning a promise on init and then resolving/rejecting it)?

bradrich · 2016-10-12T12:53:13Z

src/components/panel/panel.js

@@ -126,6 +126,9 @@ angular
 *     outside the panel to close it. Defaults to false.
 *   - `escapeToClose` - `{boolean=}`: Whether the user can press escape to
 *     close the panel. Defaults to false.
+ *   - `postClose` - `{function=}`: Function that is called after close


I think naming this onCloseSuccess would be more appropriate.

bradrich · 2016-10-12T12:55:57Z

src/components/panel/panel.js

@@ -1153,20 +1156,24 @@ MdPanelRef.prototype.open = function() {

 /**
 * Closes the panel.
+ * @param {string} closeEvent The event type that triggered the close.


This parameter would most likely not be a string. Events are normally MouseEvent, or AngularEvent.

You should probably not mention "event" here, since that means something very different to most people, as @bradrich mentioned. Perhaps, closeReason, closeCause, or even just "reason" or "cause" would work.

@ErinCoughlan Good catch! I was thinking that it was going to be the event that the occurred to get there, not a string representation.

bradrich · 2016-10-12T12:57:04Z

src/components/panel/panel.spec.js

@@ -1043,6 +1043,7 @@ describe('$mdPanel', function() {
        clickPanelContainer();

        expect(myButton).toBeFocused();
+


Why the extra lines here?

bradrich · 2016-10-12T13:02:00Z

src/components/panel/panel.spec.js

      });
    });
  });

+  describe('postClose logic:', function() {
+      it('postClose when provided', function () {


These tests should have more descriptive names. See the rest of the it tests, they all have the form of should do something when something.... These tests are failing in Travis and it wasn't immediately apparent without reading the tests what the test was trying to accomplish.

Follow the same test naming as the other events we fire, like onOpenComplete or onRemoving.

ErinCoughlan · 2016-10-12T17:38:45Z

src/components/panel/panel.js

@@ -126,6 +126,9 @@ angular
 *     outside the panel to close it. Defaults to false.
 *   - `escapeToClose` - `{boolean=}`: Whether the user can press escape to
 *     close the panel. Defaults to false.
+ *   - `postClose` - `{function=}`: Function that is called after close
+ *     is done. The first parameter passed into this function is the closeEvent
+ *     and the 2nd is the current panelRef.


I thought panelRef was first because that has to be defined and closeEvent is optional.

You should also mention the two events for pressEscapeToClose and clickBackdropToClose or no one will know how to use them.

ErinCoughlan · 2016-10-12T17:39:26Z

src/components/panel/panel.js

@@ -126,6 +126,9 @@ angular
 *     outside the panel to close it. Defaults to false.
 *   - `escapeToClose` - `{boolean=}`: Whether the user can press escape to
 *     close the panel. Defaults to false.
+ *   - `postClose` - `{function=}`: Function that is called after close


{function(panelRef, string)=} is the correct type, especially since this has params.

ErinCoughlan · 2016-10-12T17:41:24Z

src/components/panel/panel.js

@@ -126,6 +126,9 @@ angular
 *     outside the panel to close it. Defaults to false.
 *   - `escapeToClose` - `{boolean=}`: Whether the user can press escape to
 *     close the panel. Defaults to false.
+ *   - `postClose` - `{function=}`: Function that is called after close
+ *     is done. The first parameter passed into this function is the closeEvent


Possible re-word to "after close has successfully finished". "done" is too vague for this, since it will not be called if the panel fails the close.

ErinCoughlan · 2016-10-12T17:43:51Z

src/components/panel/panel.js

@@ -1153,20 +1156,24 @@ MdPanelRef.prototype.open = function() {

 /**
 * Closes the panel.
+ * @param {string} closeEvent The event type that triggered the close.


You should probably not mention "event" here, since that means something very different to most people, as @bradrich mentioned. Perhaps, closeReason, closeCause, or even just "reason" or "cause" would work.

ErinCoughlan · 2016-10-12T17:47:09Z

src/components/panel/panel.js

@@ -1704,7 +1711,7 @@ MdPanelRef.prototype._configureEscapeToClose = function() {
        ev.stopPropagation();
        ev.preventDefault();

-        self.close();
+        self.close("escapeToClose");


There should be an enum that the user can access for the different close causes (like the enum for positioning or animations).

String values are dangerous because it allows for typos.

ErinCoughlan · 2016-10-12T17:48:48Z

src/components/panel/panel.spec.js

      });
    });
  });

+  describe('postClose logic:', function() {
+      it('postClose when provided', function () {


Follow the same test naming as the other events we fire, like onOpenComplete or onRemoving.

ErinCoughlan · 2016-10-12T17:51:00Z

src/components/panel/panel.spec.js

+  describe('postClose logic:', function() {
+      it('postClose when provided', function () {
+        var obj = {
+          postClose: function() {},


Why do you need to make an object here? Does it suffice to just create a spy, then pass it to the config?

var onCloseSuccess = jasmine.createSpy('onCloseSuccess'); var config = angular.extend({ 'postClose': onCloseSuccess }); openPanel(config); closePanel(); expect(onCloseSuccess).toHaveBeenCalledWith(panelRef, undefined);

DerekLouie · 2016-10-13T01:22:56Z

Hello! All the aforementioned comments have been addressed, please take another look at this PR at your earliest convenience :)

bradrich · 2016-10-13T11:56:32Z

LGTM

ErinCoughlan · 2016-10-13T14:46:06Z

src/components/panel/panel.js

@@ -126,6 +126,12 @@ angular
 *     outside the panel to close it. Defaults to false.
 *   - `escapeToClose` - `{boolean=}`: Whether the user can press escape to
 *     close the panel. Defaults to false.
+ *   - `onCloseSuccess` - `{function(panelRef, string)=}`: Function that is


ErinCoughlan · 2016-10-13T14:47:54Z

src/components/panel/panel.js

+ *   - `onCloseSuccess` - `{function(panelRef, string)=}`: Function that is
+ *     called after the close successfully finishes. The first parameter passed
+ *     into this function is the current panelRef and the 2nd is an optional
+ *     string explaining the close reason. Currently 'pressEscapeToClose' and


Aren't these enums? that the user can get access to?

ErinCoughlan · 2016-10-13T14:49:45Z

src/components/panel/panel.js

+ * Possible default closeReasons for the close function.
+ * @enum {string}
+ */
+MdPanelRef.closeReasons = {


You'll want this to be exported on the mdPanel service as well so users can access them. (Follow what the other enums did.)

ErinCoughlan · 2016-10-13T14:52:51Z

src/components/panel/panel.spec.js

@@ -1059,6 +1059,77 @@ describe('$mdPanel', function() {
    });
  });

+  describe('onCloseSuccess logic:', function() {


I don't think these tests should be it's own group. This functionality is part of the close logic, and therefore should be individual tests after the other close callback tests.
Like this:
https://github.com/angular/material/blob/master/src/components/panel/panel.spec.js#L877

ErinCoughlan · 2016-10-13T14:53:25Z

src/components/panel/panel.spec.js

+            clickPanelContainer();
+
+            expect(onCloseSuccessCalled).toBe(true);
+            expect(closeReason).toBe('clickOutsideToClose');


Use the enums in the tests too, not strings.

DerekLouie · 2016-10-13T18:45:40Z

Fixed the enums / documentation comments. Please take another look at your earliest convenience :).

ErinCoughlan

LGTM

DerekLouie added the EOC label Oct 12, 2016

DerekLouie self-assigned this Oct 12, 2016

bradrich suggested changes Oct 12, 2016

View reviewed changes

ErinCoughlan suggested changes Oct 12, 2016

View reviewed changes

ThomasBurleson added in progress Mainly for in progress PRs, but may be used for issues that require multiple PRs needs: work labels Oct 12, 2016

ThomasBurleson added this to the 1.1.3 milestone Oct 12, 2016

DerekLouie force-pushed the panelAnimationHooks branch from e204658 to 85fa302 Compare October 13, 2016 01:22

bradrich approved these changes Oct 13, 2016

View reviewed changes

ErinCoughlan suggested changes Oct 13, 2016

View reviewed changes

DerekLouie force-pushed the panelAnimationHooks branch 2 times, most recently from d6dc99a to 40e534c Compare October 13, 2016 18:44

DerekLouie force-pushed the panelAnimationHooks branch from 40e534c to 132c106 Compare October 13, 2016 18:51

ErinCoughlan approved these changes Oct 13, 2016

View reviewed changes

ErinCoughlan removed the EOC label Oct 13, 2016

feat(panel): Adding a hook on close success.

5e6eeb5

DerekLouie force-pushed the panelAnimationHooks branch from 132c106 to 5e6eeb5 Compare October 17, 2016 19:30

ErinCoughlan added needs: presubmit and removed in progress Mainly for in progress PRs, but may be used for issues that require multiple PRs needs: work labels Oct 18, 2016

ThomasBurleson modified the milestones: 1.1.2, 1.1.3 Oct 18, 2016

jelbourn merged commit db90283 into angular:master Oct 24, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(panel): Adding a hook on close success. #9819

feat(panel): Adding a hook on close success. #9819

DerekLouie commented Oct 12, 2016

crisbeto commented Oct 12, 2016 •

edited

ErinCoughlan commented Oct 12, 2016

crisbeto commented Oct 12, 2016

bradrich Oct 12, 2016 •

edited

bradrich Oct 12, 2016

ErinCoughlan Oct 12, 2016

bradrich Oct 13, 2016

bradrich Oct 12, 2016

bradrich Oct 12, 2016

ErinCoughlan Oct 12, 2016

ErinCoughlan Oct 12, 2016

ErinCoughlan Oct 12, 2016

ErinCoughlan Oct 12, 2016

ErinCoughlan Oct 12, 2016

ErinCoughlan Oct 12, 2016

ErinCoughlan Oct 12, 2016

ErinCoughlan Oct 12, 2016

DerekLouie commented Oct 13, 2016

bradrich commented Oct 13, 2016

ErinCoughlan Oct 13, 2016

ErinCoughlan Oct 13, 2016

ErinCoughlan Oct 13, 2016

ErinCoughlan Oct 13, 2016

ErinCoughlan Oct 13, 2016

DerekLouie commented Oct 13, 2016

ErinCoughlan left a comment

		@@ -1043,6 +1043,7 @@ describe('$mdPanel', function() {
		clickPanelContainer();

		expect(myButton).toBeFocused();

feat(panel): Adding a hook on close success. #9819

feat(panel): Adding a hook on close success. #9819

Conversation

DerekLouie commented Oct 12, 2016

crisbeto commented Oct 12, 2016 • edited

ErinCoughlan commented Oct 12, 2016

crisbeto commented Oct 12, 2016

bradrich Oct 12, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DerekLouie commented Oct 13, 2016

bradrich commented Oct 13, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DerekLouie commented Oct 13, 2016

ErinCoughlan left a comment

Choose a reason for hiding this comment

crisbeto commented Oct 12, 2016 •

edited

bradrich Oct 12, 2016 •

edited