Add config option for dropdown to not close on $locationChangeSuccess

[rmckeel]

Howdy, this is an excellent library - thanks for your hard work on it!

I've run across a feature (really, a configuration option) I think would be helpful to add. We have something that constantly updates $location.search with a new hash, e.g. one moment it will be myPage?hash=1234 then the next moment it will change to myPage?hash=5678.

Unless auto-close on dropdowns is disabled, it will automatically close the dropdowns. I was working around this by having something watch and re-open the dropdowns, but this is certainly not ideal. I'd prefer not to maintain a fork with only this feature, and I think this will be useful for others to have this as an option. Updating the location is a fairly normal thing these days, and if it is a page location change (such as in a single page application), Angular would already destroy the controller and directive, meaning this code doesn't seem to help too much in normal cases (but perhaps I'm not normal and not thinking of normal cases ;)

What would you think of a config option to ignore $locationChangeSuccess? I could create a PR given some simple guidance (such as what / how you'd like the option called).

Here's the relevant code in dropdown.js

  $scope.$on('$locationChangeSuccess', function() {
    if (scope.getAutoClose() !== 'disabled') {
      scope.isOpen = false;
    }
  });

[icfantv]

@rmckeel, i think the bigger question is why you have a hash on your page URLs.

[wesleycho]

The whole point of the is-open binding is to allow users to control the condition for when dropdowns open/close.

Closing as a feature that is not desired in the library itself, too user-specific & already possible without much hassle on the user side.

[rmckeel]

@icfantv I have a hash on page URLs to enable back/forward button and link sharing; the user modifies the app state, then the hash updates. Additionally, hashes can be used to track auto-save states. This type of auto-updated hash feature has been in use by GMail for years (watch the URL bar as you type an email), so using hashes to track app state / save status is not too odd if used in an app used by millions daily.

@wesleycho Wesley, I don't know of a way to intercept $locationChangeSuccess cleanly, and the options for the auto-close are not sufficient (this app needs an outsideClick without tracking $locationChangeSuccess). I suppose I'll just fork and modify it (do the "hack core" method), I thought it would be helpful for others though as an option.

Thanks

[calebergh]

This feature request makes sense, and would help me with my project.

[icfantv]

@rmckeel - that's an interesting use case. I feel compelled to point out that Gmail is maintained by a team of paid engineers at Google who do that work full time. We are all unpaid volunteers and they're about 5-6 of us right now on UIBS who do the work in our spare time (in addition to our full time jobs of working, family, kids, etc...).

All that said, the reason we've added the is-open attribute is to allow un-opinionated delegation of the logic to you, the developer. If you want this behavior, the correct way is to create a wrapper directive to trap the location change event and prevent it from being broadcast down further.

[rmckeel]

@icfantv Hey, thanks for your response. Yes, I understand limited time all too well - and that's what I was saying I could make the change / PR. However, it appears to not be of interest as an additional setting, or auto-close="outsideClickNoLocationChange" or some such. Understandable.

Thanks - I hadn't thought of a wrapper directive to prevent a broadcast from further trickling down - great idea! Clean, doesn't require forking the repo and keeping that updated, doesn't require my hack (where I use is-open and change state back to what it was just after it closes).

Thanks for the idea on wrapper directive - I'll give that a go. Cheers and thanks for your hard work on this.

[icfantv]

And it's reusable! :-)

And you're welcome.

[rmckeel]

@icfantv Hey Adam, I've tried this, and finally found the Angular documentation comment on $broadcast: "The event cannot be canceled." A wrapper directive can toggle event.defaultPrevented, but it can't keep a $broadcast from trickling down further.

So, back to square one. Fork it or offer again to make it a minor additional option (or track states of all the dropdowns via is-open and change it right after a $locationChangeSuccess, which isn't pretty but works).

Any interest in me making a small config option and issuing a PR? Or do you still think no one else cares to have 'outsideClick' functionality that doesn't collapse on $locationChangeSuccess? It seems to me a fringe case the way it is currently developed, since Angular already destroys the directive on location change (thus closing it), but I defer to the dev team. Just trying not to hack this awesome directive if possible.

[rmckeel]

Another quick thought @wesleycho and @icfantv . If this is a fringe case (but one that happens), how about giving developers the ability to flag the $broadcast as defaultPrevented? e.g. this simple change would give control back to developers to prevent the event from being responded to:

  $scope.$on('$locationChangeSuccess', function(evt) {
    if (scope.getAutoClose() !== 'disabled' && !evt.defaultPrevented) {
      scope.isOpen = false;
    }
  });

[wesleycho]

The thing is, this is not the responsibility of the dropdown itself - we try to follow the single responsibility principle as much as possible, and this is not really in the realm of the dropdown itself, but whatever layer controls the desired business logic for responding to this angular event.

How is this not accomplishable using a wrapper directive currently?

[rmckeel]

Hi @wesleycho, good point on the single responsibility principle.

Please see Angular documentation as referenced, there is a documentation comment on $broadcast: "The event cannot be canceled." Thus, as far as I understand (and from my testing of a sample wrapper directive I built), a wrapper directive can toggle event.defaultPrevented, but it can't stop a $broadcast from trickling down further. It is up to whatever code creates the listener to check if event.defaultPrevented is true.

Unfortunately, without modifying the dropdown code (or substituting for a non-ideal auto-close setting such as 'disabled'), there is no way to prevent the $locationChangeSuccess message from getting to it.

If you want, I can create a demonstration plnkr, but I'm hoping the Angular documentation is enough.

Thanks Wesley!

[wesleycho]

We don't have any $locationChangeSuccess specific code in the library anymore, so I'm a bit confused as to the point being made here.

[wesleycho]

Oh, I stand corrected - we should remove that code.

[rmckeel]

Ah, great! OK, that would solve things. I'm guessing that is too small of a change to have value in me creating a PR? Happy to create a PR if that would help you out.

[wesleycho]

If you want to do so, go for it!

[rmckeel]

Sure! I'll get that done later today (a PR that removes the entire block
of $locationChangeSuccess code) and update this thread. Thanks for your
assistance,

Ryan

On Tue, Mar 22, 2016 at 9:25 AM, Wesley Cho [email protected]
wrote:

If you want to do so, go for it!

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#5648 (comment)

[rmckeel]

Just created a PR, took my best stab at removing from test specs also.