Skip to content
This repository has been archived by the owner on May 29, 2019. It is now read-only.

feat(modal): Adds defaultContainer to $modalProvider #3516

Closed
wants to merge 1 commit into from

Conversation

Lalem001
Copy link

@Lalem001 Lalem001 commented Apr 8, 2015

Includes basic tests, with adjustments to toHaveModalsOpen and toHaveBackdrop.

Fixes: #2688
Replaces PR: #2942

@@ -454,6 +456,7 @@ angular.module('ui.bootstrap.modal', [])
backdropClass: modalOptions.backdropClass,
windowClass: modalOptions.windowClass,
windowTemplateUrl: modalOptions.windowTemplateUrl,
container: $modalProvider.defaultContainer,
Copy link
Contributor

Choose a reason for hiding this comment

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

Looking at the original issue this PR references, any particular reason for the choice of container over appendTo? I think the latter makes more sense semantically, but am open to other options.

Copy link
Author

Choose a reason for hiding this comment

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

container is just coming off of my use of defaultContainer. Would you want appendTo and defaultContainer? Or change both to appendTo and defaultAppendTo?

Or perhaps something else entirely?

Copy link
Contributor

Choose a reason for hiding this comment

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

It should be appendTo and there should be no defaultContainer reference.

@wesleycho wesleycho modified the milestones: 0.14.3, 1.0.0 Oct 23, 2015
@wesleycho
Copy link
Contributor

Should be noted, we now have an appendTo feature via 16d854c - this feature should interop with the new implementation. Given that it takes a DOM node in the $modal service, I think this feature then becomes invalid. If one needs a sensible default, I recommend creating a modal abstraction on top of $modal to pass the desired defaults.

@wesleycho wesleycho closed this Nov 2, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

appendTo option for modal dialog
3 participants