fix(panel): clickOutsideToClose with propagateContainerEvents

[bradrich]

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]

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]

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

[ErinCoughlan]

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]

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

[bradrich]

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]

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.

[bradrich]

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.

[crisbeto]

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

[ErinCoughlan]

@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.

[crisbeto]

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]

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

[bradrich]

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

[crisbeto]

Alright, it makes sense without the var.

[bradrich]

@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]

@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]

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

[bradrich]

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

[bradrich]

@ThomasBurleson This is ready for merge.

[bradrich]

@ThomasBurleson Checking on the status of this PR.