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

feat(modal): add container option #4599

Closed
wants to merge 1 commit into from
Closed

feat(modal): add container option #4599

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/modal/docs/readme.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ The `$uibModal` service has only one method: `open(options)` where available opt
* `windowTemplateUrl` - a path to a template overriding modal's window template
* `size` - optional suffix of modal window class. The value used is appended to the `modal-` class, i.e. a value of `sm` gives `modal-sm`
* `openedClass` - class added to the `body` element when the modal is opened. Defaults to `modal-open`
* `container` - Appends the modal to a specific element. container must be an `angular.element`. Defaults to `body` element. Example: `container: $document.find('aside')`.

Global defaults may be set for `$uibModal` via `$uibModalProvider.options`.

Expand Down
25 changes: 16 additions & 9 deletions src/modal/modal.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -276,16 +276,16 @@ angular.module('ui.bootstrap.modal', ['ui.bootstrap.stackedMap'])
});

function removeModalWindow(modalInstance, elementToReceiveFocus) {
var body = $document.find('body').eq(0);
var modalWindow = openedWindows.get(modalInstance).value;
var container = $document.find(modalWindow.container).eq(0);

//clean up the stack
openedWindows.remove(modalInstance);

removeAfterAnimate(modalWindow.modalDomEl, modalWindow.modalScope, function() {
var modalBodyClass = modalWindow.openedClass || OPENED_MODAL_CLASS;
openedClasses.remove(modalBodyClass, modalInstance);
body.toggleClass(modalBodyClass, openedClasses.hasKey(modalBodyClass));
container.toggleClass(modalBodyClass, openedClasses.hasKey(modalBodyClass));
toggleTopWindowClass(true);
});
checkRemoveBackdrop();
Expand All @@ -294,7 +294,7 @@ angular.module('ui.bootstrap.modal', ['ui.bootstrap.stackedMap'])
if (elementToReceiveFocus && elementToReceiveFocus.focus) {
elementToReceiveFocus.focus();
} else {
body.focus();
container.focus();
}
}

Expand Down Expand Up @@ -413,14 +413,19 @@ angular.module('ui.bootstrap.modal', ['ui.bootstrap.stackedMap'])
backdrop: modal.backdrop,
keyboard: modal.keyboard,
openedClass: modal.openedClass,
windowTopClass: modal.windowTopClass
windowTopClass: modal.windowTopClass,
container: modal.container
});

openedClasses.put(modalBodyClass, modalInstance);

var body = $document.find('body').eq(0),
var container = $document.find(modal.container).eq(0),
Copy link
Contributor

Choose a reason for hiding this comment

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

what happens if modal.container refers to a container that doesn't exist?

Copy link
Contributor

Choose a reason for hiding this comment

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

This is not going to work. jqLite is strictly limited to finding by tag name so what's the use of being able to specify something other than <body> here?

Copy link
Contributor

Choose a reason for hiding this comment

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

modal.container should probably default to $document.find('body'), but allow the user to link to a jqLite wrapped DOM node for the api.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@icfantv

Now I am throwing the following error is container is not found

Container not found. jqLite is limited to lookups by tag name. Make sure that the container passed is in DOM.

@wesleycho

Good idea. I can change to that approach if we all agree

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm ok with this. The documentation should state that the value of of the container attribute is an angular.element.

Copy link
Contributor

Choose a reason for hiding this comment

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

I still think we need to only grab the first one. If I'm reading this right, if someone passed in multiple objects, the modal would get appended to each of them.

Copy link
Contributor

Choose a reason for hiding this comment

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

Does jqLite support it? That is my main concern

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry. Support what? jqLite supports both .eq() and .append() with no strings attached (according to this).

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, thought for some reason Angular didn't support .eq in jqLite for a while - looks like I was wrong.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess logging error makes more sense here as user has explicitly passed the invalid container element. Let me know if you want me to default it to body.

currBackdropIndex = backdropIndex();

if (!container.length) {
Copy link
Contributor

Choose a reason for hiding this comment

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

We shouldn't mention jqLite because if the user includes the full jQuery library before Angular, Angular will use the full jQuery library. This goes back to how we want to do error handling as mentioned above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated error message to Container not found. Make sure that the container passed is in DOM.

throw new Error('Container not found. Make sure that the container passed is in DOM.');
}

if (currBackdropIndex >= 0 && !backdropDomEl) {
backdropScope = $rootScope.$new(true);
backdropScope.index = currBackdropIndex;
Expand All @@ -430,7 +435,7 @@ angular.module('ui.bootstrap.modal', ['ui.bootstrap.stackedMap'])
angularBackgroundDomEl.attr('modal-animation', 'true');
}
backdropDomEl = $compile(angularBackgroundDomEl)(backdropScope);
body.append(backdropDomEl);
container.append(backdropDomEl);
}

var angularDomEl = angular.element('<div uib-modal-window="modal-window"></div>');
Expand All @@ -449,8 +454,8 @@ angular.module('ui.bootstrap.modal', ['ui.bootstrap.stackedMap'])
var modalDomEl = $compile(angularDomEl)(modal.scope);
openedWindows.top().value.modalDomEl = modalDomEl;
openedWindows.top().value.modalOpener = modalOpener;
body.append(modalDomEl);
body.addClass(modalBodyClass);
container.append(modalDomEl);
container.addClass(modalBodyClass);

$modalStack.clearFocusListCache();
};
Expand Down Expand Up @@ -603,6 +608,7 @@ angular.module('ui.bootstrap.modal', ['ui.bootstrap.stackedMap'])
//merge and clean up options
modalOptions = angular.extend({}, $modalProvider.options, modalOptions);
modalOptions.resolve = modalOptions.resolve || {};
modalOptions.container = modalOptions.container || 'body';

//verify options
if (!modalOptions.template && !modalOptions.templateUrl) {
Expand Down Expand Up @@ -669,7 +675,8 @@ angular.module('ui.bootstrap.modal', ['ui.bootstrap.stackedMap'])
windowClass: modalOptions.windowClass,
windowTemplateUrl: modalOptions.windowTemplateUrl,
size: modalOptions.size,
openedClass: modalOptions.openedClass
openedClass: modalOptions.openedClass,
container: modalOptions.container
});
modalOpenedDeferred.resolve(true);

Expand Down
42 changes: 42 additions & 0 deletions src/modal/test/modal.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -833,6 +833,48 @@ describe('$uibModal', function () {
expect($document.find('.modal-backdrop')).not.toHaveClass('fade');
});
});

describe('container', function() {
it('should be added to body by default', function() {
var modal = open({template: '<div>Content</div>'});

expect($document).toHaveModalsOpen(1);
expect($document).toHaveModalOpenWithContent('Content', 'div');
});

it('should not be added to body if container is passed', function() {
var element = angular.element('<section>Some content</section>');
angular.element(document.body).append(element);

var modal = open({template: '<div>Content</div>', container: element});

expect($document).not.toHaveModalOpenWithContent('Content', 'div');
Copy link
Contributor

Choose a reason for hiding this comment

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

it is not sufficient to test that the body doesn't have a modal. you need to check that the modal is on the component you've specified and only on that component.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. Added couple of more test cases

});

it('should be added to container if container is passed', function() {
var element = angular.element('<section>Some content</section>');
angular.element(document.body).append(element);

expect($document.find('section').children('div.modal').length).toBe(0);
open({template: '<div>Content</div>', container: element});
expect($document.find('section').children('div.modal').length).toBe(1);
});

it('should throw error if container is not found', function() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Still not sure how we want to handle the error. If we throw something, should we display a link to an error page like core Angular does (and other Angular libraries)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think we should handle this error as there is a need for the user to fix the expectation. Let me know if you want me to point the error to docs page.

expect(function(){
open({template: '<div>Content</div>', container: $document.find('aside')});
}).toThrow(new Error('Container not found. Make sure that the container passed is in DOM.'));
});

it('should be removed from container when dismissed', function() {
var modal = open({template: '<div>Content</div>'});

expect($document).toHaveModalsOpen(1);

dismiss(modal);
expect($document).toHaveModalsOpen(0);
});
});

describe('openedClass', function() {
var body;
Expand Down