fix(panel): clickOutsideToClose with propagateContainerEvents #9886

bradrich · 2016-10-21T18:52:23Z

If both the clickOutsideToClose and propagateContainerEvents parameters are set to true within the panel configuration, then the panel will be closed when a click happens outside of the panel.

Fixes #9388

ErinCoughlan · 2016-10-21T19:37:51Z

src/components/panel/panel.js

+      var found = false;
+      angular.forEach(elems, function(elem) {
+        if (elem === sourceElem) {
+          found = true;


How about this? It quits early when it finds a match and removes the extra variable.

var findSourceEl = function(els, sourceEl) { for (var el, i = 0; el = els[i]; i++) { if (el === sourceEl) { return true; } }); return false; };

ErinCoughlan · 2016-10-21T19:38:26Z

src/components/panel/panel.js

+
+        // We check if the sourceElem of the event is the panel element or one
+        // of it's children. If it is not, then close the panel.
+        var found = findSourceElem(


El instead of Elem. We use el throughout this file, so it's strange to switch naming conventions here.

ErinCoughlan · 2016-10-21T19:49:23Z

src/components/panel/panel.spec.js

+
+          openPanel(config);
+
+          clickOutsideOfPanel();


Can this be clickPanelContainer? Or can clickPanelContainer be updated so you're not duplicating logic. (The only thing that's changing is the element you're clicking on. Maybe something like this?

function clickPanelContainer(rootEl) { rootEl = rootEl || document.body; const selector = '.md-panel:not(._md-panel-backdrop)'; angular.element(rootEl).find(selector).parent().simulate('click'); angular.element(rootEl).find(selector).parent().simulate('mousedown'); angular.element(rootEl).find(selector).parent().simulate('mouseup'); flushPanel(); }

Benefits of this version is that you don't need the panelRef, which removes one of the if statements. I know this version can replace the current one and you won't need to change any tests.

or

function clickPanelContainer(container) { container = container || angular.element('.md-panel-outer-wrapper'); container.triggerHandler({ type: 'mousedown', target: container[0] }); container.triggerHandler({ type: 'mouseup', target: container[0] }); flushPanel(); }

crisbeto · 2016-10-22T08:32:44Z

src/components/panel/panel.js

+
+        // We check if the sourceEl of the event is the panel element or one
+        // of it's children. If it is not, then close the panel.
+        var found = findSourceEl(


You could simplify it and avoid the loop if you did something like this:

var found = sourceEl === self.panelEl[0] || self.panelEl[0].contains(sourceEl);

I was looking for the .contains() method and didn't realize that I would have to access the element to get to it. Good catch and I appreciate you showing it to me.

crisbeto · 2016-10-22T08:35:40Z

src/components/panel/panel.spec.js

+            clickOutsideToClose: true
+          };
+
+          openPanel(config);


It might make the test slightly more robust if you had a check that the PANEL_EL exists, right after opening it. This should avoid cases where the test will pass, if the openPanel function didn't open it up for some reason.

The open panel result is pretty well tested in the tests that proceed this one. You will also notice in the test right before this one, should close when clickOutsideToClose set to true does not expect the PANEL_EL to exist before it expects it to be closed. I went with that understanding to write this one.

While it's true that the opening logic is well tested, we should try to keep the tests as independent as possible.

@crisbeto I go back and forth on this one. Because the open existence is tested earlier, we get a better test signal when opens fails because the failure is in the test that tests open. If we write it in every test, then all will fail and the messages don't point to the actual problem.

I wouldn't say it's necessary for every test, but it makes sense for this one since the test would fail if there isn't an open panel. This may be down to personal preference, though.

crisbeto · 2016-10-22T08:38:38Z

src/components/panel/panel.spec.js

    if (!panelRef) {
      return;
    }

-    var container = panelRef.panelContainer;
+    var container = container || panelRef.panelContainer;


It's a bad practise to have variables with the same name as function arguments.

This should actually read as container = container || panelRef.panelContainer. I forgot to remove the var before it.

Alright, it makes sense without the var.

@ThomasBurleson I think that there was some confusion as we extended this function. Previously, the container variable was declared internally and assigned to the panelRef.panelContainer. Now, you are able to pass in a container to click on. But, if you don't pass one in, it needs to still assign it to the default panelRef.panelContainer.

crisbeto

LGTM 👍

ErinCoughlan

LGTM

ErinCoughlan · 2016-10-24T17:39:43Z

src/components/panel/panel.spec.js

+            clickOutsideToClose: true
+          };
+
+          openPanel(config);


@crisbeto I go back and forth on this one. Because the open existence is tested earlier, we get a better test signal when opens fails because the failure is in the test that tests open. If we write it in every test, then all will fail and the messages don't point to the actual problem.

ErinCoughlan · 2016-10-24T17:43:53Z

src/components/panel/panel.spec.js

+
+          openPanel(config);
+
+          clickPanelContainer(angular.element(document.body));


Can you move the angular.element part to the clickPanelContainer? I don't think it particularly matters for this test, so it seems to fit better in the helper method.

I'm wondering if we want something like getElement to make this more flexible.

material/test/angular-material-spec.js

Line 287 in 72d0685

function getElement(el) {

Went ahead and added this one, as I thought that it was a more extended implementation and useful.

…Events If both the `clickOutsideToClose` and `propagateContainerEvents` parameters are set to true within the panel configuration, then the panel will be closed when a click happens outside of the panel. Fixes angular#9388

bradrich · 2016-10-24T18:35:39Z

@ThomasBurleson This is ready for merge.

bradrich · 2016-11-02T13:07:48Z

@ThomasBurleson Checking on the status of this PR.

bradrich assigned DerekLouie, ErinCoughlan and crisbeto Oct 21, 2016

ErinCoughlan reviewed Oct 21, 2016

View reviewed changes

bradrich force-pushed the fix/clickOutsideToClose-with-propagateContainerEvents branch from 639cdaf to f0ad663 Compare October 21, 2016 20:31

crisbeto suggested changes Oct 22, 2016

View reviewed changes

bradrich force-pushed the fix/clickOutsideToClose-with-propagateContainerEvents branch from f0ad663 to 360d631 Compare October 24, 2016 13:48

crisbeto approved these changes Oct 24, 2016

View reviewed changes

bradrich force-pushed the fix/clickOutsideToClose-with-propagateContainerEvents branch from 360d631 to 1f18e4c Compare October 24, 2016 14:03

ErinCoughlan approved these changes Oct 24, 2016

View reviewed changes

bradrich force-pushed the fix/clickOutsideToClose-with-propagateContainerEvents branch from 1f18e4c to 76bb9b6 Compare October 24, 2016 18:34

ThomasBurleson added the needs: presubmit label Oct 27, 2016

ThomasBurleson added this to the 1.1.2 milestone Oct 27, 2016

kara added pr: merge ready This PR is ready for a caretaker to review and removed needs: presubmit labels Nov 17, 2016

kara merged commit 61bd95e into angular:master Nov 17, 2016

angular locked as resolved and limited conversation to collaborators Mar 7, 2020

Splaktar unassigned DerekLouie and ErinCoughlan Mar 7, 2020


		openPanel(config);

		clickPanelContainer(angular.element(document.body));

fix(panel): clickOutsideToClose with propagateContainerEvents #9886

fix(panel): clickOutsideToClose with propagateContainerEvents #9886

Uh oh!

Conversation

bradrich commented Oct 21, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

crisbeto left a comment

Choose a reason for hiding this comment

Uh oh!

ErinCoughlan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bradrich commented Oct 24, 2016

Uh oh!

bradrich commented Nov 2, 2016

Uh oh!

Uh oh!