WIP - feat(popover): Add container input, support 'body' selector #853
Conversation
spongessuck
changed the title
spongessuck
changed the title
|
@spongessuck thnx for the PR, it looks like a first good step. Having said this I had a slightly different plan for this. My intention was to introduce With my proposal we would be not introducing APIs that would be redundant / deprecated in the future. Would you be willing to do the above changes in your PR? |
|
Sure, it shouldn't be hard. |
|
What about |
I would prefer '"body"' for now as it will still work when we add support for all selectors.
Awesome! |
|
I'll do "html" then. I still think it's a bad idea to call it "body" when it's not measuring from the body. |
Could we please stick to |
|
If the body has any padding or margin I don't think it will be placed properly if it's in the body. |
|
I guess it'd have to be positioned |
|
Did some tests and it doesn't seem to matter where the body is- the position service is smarter than I thought :). I'll use "body" as requested. |
spongessuck
changed the title
| @@ -95,6 +109,7 @@ export class NgbPopover implements OnInit, OnDestroy { | |||
| this._windowRef = this._popupService.open(this.ngbPopover); | |||
| this._windowRef.instance.placement = this.placement; | |||
| this._windowRef.instance.title = this.popoverTitle; | |||
| this._windowRef.instance.container = this.container; | |||
There was a problem hiding this comment.
Does popover window need to be aware of the container option?
There was a problem hiding this comment.
I guess it doesn't.
| @@ -34,6 +34,7 @@ import {NgbPopoverConfig} from './popover-config'; | |||
| export class NgbPopoverWindow { | |||
| @Input() placement: 'top' | 'bottom' | 'left' | 'right' = 'top'; | |||
| @Input() title: string; | |||
| @Input() container: string; | |||
There was a problem hiding this comment.
Why do we need this input?
| @@ -382,6 +403,7 @@ describe('ngb-popover', () => { | |||
| config = c; | |||
| config.placement = 'bottom'; | |||
| config.triggers = 'hover'; | |||
| config.container = 'body'; | |||
There was a problem hiding this comment.
Why do we need to modify this test?
There was a problem hiding this comment.
Doesn't this apply to popover and not popoverwindow? Shouldn't it test that it takes custom config properly?
There was a problem hiding this comment.
Oh, right, sorry for the noise.
| this.container === 'body'); | ||
|
|
||
| if (this.container === 'body') { | ||
| let windowEl = this._windowRef.location.nativeElement; |
There was a problem hiding this comment.
Nit: maybe drop the temporary windowEl variable?
There was a problem hiding this comment.
It's carried over from when I was using the native ref in a few more operations- I guess it can be dropped, but I think it makes it more readable.
There was a problem hiding this comment.
Whatever you prefer, I'm good either way.
| @@ -191,6 +193,25 @@ describe('ngb-popover', () => { | |||
| }); | |||
| }); | |||
|
|
|||
| describe('placement', () => { | |||
There was a problem hiding this comment.
Nit: 'placement' => 'container'?
|
Thnx for all the updates @spongessuck - it all starts to come together 👍 |
|
Not sure if you got notifs from my commits, but I think this is good to go. |
|
Thnx @spongessuck - squashed and merged as d1a22c0. Would be awesome if you could send a similar (well, almost identical) PR for tooltips :-) |
Adds an
appendToRootinput to the popover component to allow for placement of the popover in the root element. Addresses #852.I'm not sure about the cross-platform implications of this solution, but it works in the browser, is analogous to the ui-bootstrap
appendToBodyoption, and makes popover DOM placement more flexible until #339 is implemented.