feat(modal): add initial version of the modal service #609
Conversation
| @@ -16,13 +17,14 @@ import {NgbTooltipModule} from './tooltip'; | |||
| import {NgbTypeaheadModule} from './typeahead'; | |||
|
|
|||
| export {NgbPanelChangeEvent} from './accordion'; | |||
| export {NgbModal} from './modal'; | |||
There was a problem hiding this comment.
Shouldn't NgbModalRef and NgbModalOptions be exported too? They're the return type and the argument type of NgbModal.open(), which is the public API, so client code should be able to refer to these types, too, shouldn't they?
There was a problem hiding this comment.
They should, I thought I've exported them. This is still WIP so thx for catching this one!
pkozlowski-opensource
changed the title
| }); | ||
| } | ||
|
|
||
| private _getContentNodes(content: string | TemplateRef<any>, context: ModalContentContext) { |
There was a problem hiding this comment.
What type of object is this returning?
There was a problem hiding this comment.
An array of DOM nodes. Will add return type.
|
This LGTM - it's possible I missed something, so one other person reviewing probably would be a good idea. |
| this.modalService.open(content).result.then((result) => { | ||
| this.closeResult = `Closed with: ${result}`; | ||
| }, (reason) => { | ||
| this.closeResult = `Dismissed with: ${reason}`; |
There was a problem hiding this comment.
escape and backdrop click return 1 and 0 as dismiss reasons which are NgbDismissReasons values. But it looks strange in the demo → should switch over enum values and display text?
|
LGTM, some comments in the code plus:
|
| const nodes = this._getContentNodes(content, modalContentContext); | ||
| const windowCmptRef = this._viewContainerRef.createComponent(this._windowFactory, 0, this._injector, nodes); | ||
| let backdropCmptRef; | ||
| let ngbModalRef; |
There was a problem hiding this comment.
Can't we type this var and the previous one?
|
Nothing against, minus a minimal nitpick. |
Very rough implementation of the
NgbModalservice. Opening this PR early on so we can discuss the details. Check discussion in #3 (comment) for more details / remaining work.Even if there is still more work that needs to be done I believe that this is the first good shoot witch should have stable API (I mean, remaining work shouldn't have non-backward compatible API changes). As such I think that we can go ahead and land it so users can play with it.
Please review.