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

Fix #90 and #71 at the expense of $element injection #163

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

ipavlic
Copy link

@ipavlic ipavlic commented Jun 22, 2016

If the controller is instantiated after compiling and linking, any directives included in the model template won't have access to controller input values.

For example, take a modal template which includes a foo directive, <foo bar="vm.inputValue"></foo>. If compile and link functions are executed first, then foo's controller is instantiated during before compilation.

However, since the modal controller is instantiated in code after the linking, the inputValue is undefined. This makes it impossible to use directives/components which require values from the modal controller.

@coveralls
Copy link

Coverage Status

Coverage decreased (-1.8%) to 84.127% when pulling 5e168fe on ipavlic:master into 2c07464 on dwmkerr:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-1.8%) to 84.127% when pulling 74f10c7 on ipavlic:master into 2c07464 on dwmkerr:master.

@sirLisko
Copy link

sirLisko commented Dec 5, 2016

I can confirm that it solved my problem too.
I almost got crazy trying to understand what it was going on with my directive and luckily I found @ipavlic PR.

@dwmkerr can you please merge this PR?

Cheers!

@nenesitooo
Copy link

👍

1 similar comment
@zaparka
Copy link

zaparka commented Dec 6, 2016

👍

@sirLisko
Copy link

Hey @ipavlic I found a little problem in your PR.
Basically the $element is not passed anymore.

app.controller('ExampleModalController', [
  '$scope', '$element', 'close',

I know it's a smaller issue than the one that is fixing but it could be useful to keep track also on this :)

@ipavlic
Copy link
Author

ipavlic commented Dec 15, 2016

@sirLisko Yes, that's why the commit message says "at the expense of $element injection"

@sirLisko
Copy link

@ipavlic my bad, I didn't read it properly 😅

@dwmkerr
Copy link
Owner

dwmkerr commented May 20, 2017

@ipavlic @sirLisko just to confirm, if this means that $element is not injected, then it will cause problems. In many cases people are using this library for bootstrap modals, so it is not uncommon to have functions which interact with the element, e.g:

$scope.cancel = function() {
    $element.modal('hide');
}

But I do see the issue and think that dropping element is still probably the better solution. Two questions though:

  1. How are angular doing it? They don't have the binding issues but still inject $element.
  2. What is the alternative if someone still needs the element in their controller (to support an easy migration path)

With respect to point 1, it might be that angular are not doing it in all cases (see angular/angular.js#6988).

If there seems to be no way around this and still have the element injected, we might just have to go ahead anyway and warn users that the API has changed.

kippsterr added a commit to kippsterr/angular-modal-service that referenced this pull request Jun 19, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants