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 invalid container #23

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

Conversation

xtang
Copy link

@xtang xtang commented Jul 26, 2014

hi,

I found container would be invalid when route changed, so I just select it every time when config.container is set by user. Does that make sense?

thanks.

@@ -19,6 +19,7 @@ factory('btfModal', function ($animate, $compile, $rootScope, $controller, $q, $
controllerAs = config.controllerAs,
container = angular.element(config.container || document.body),
element = null,
container = angular.element(document.body),
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if you're changing this logic, you should remove the other container = angular.element(config.container || document.body),

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, forgot that:(

@btford
Copy link
Owner

btford commented Jul 29, 2014

I don't really understand the problem. This might be a good fix, but this needs an accompanying test case showing where the existing logic falls short.

@xtang
Copy link
Author

xtang commented Jul 30, 2014

@btford Sure, I will add a test case.

@xtang
Copy link
Author

xtang commented Jul 30, 2014

I think it's not the issue of btfModal, but my usage.

I followed the example in README, because the MyModal is singleton, the container seems to be binding to a block in the first directive(template) who injects the MyModal.

When I jump into the others(also injects the MyModal), MyModal's container is still in first directive element. Hope I make it clear...

thanks

@btford
Copy link
Owner

btford commented Mar 4, 2015

Makes sense. I'm not particularly opposed to this change, but it seems like it might be better to just create a new modal instance for each controller.

@btford btford closed this Mar 4, 2015
@btford btford reopened this Mar 4, 2015
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.

2 participants