popover: extends screen width in ltr & rtl

[aghArdeshir]

Bug description:

Popover on left & Popover on right extends body, hence adding more width and make page scroll-able.
It is visible on ng-bootstrap's demo Stackblitz of Popover (on right) and this one that I prepared:
https://stackblitz.com/edit/angular-gtkkbw

Here, if you click Popover on left it extends the body (note that direction is rtl)

And this is the demo provided on Popover by ng-bootstrap: https://stackblitz.com/angular/lvnmleonenm

Note that you shouldn't make the preview pane bigger to make this happen.

Link to minimally-working StackBlitz that reproduces the issue:

https://stackblitz.com/edit/angular-gtkkbw (click on popover left)

Versions of Angular, ng-bootstrap and Bootstrap:

Angular: 7.0.0

ng-bootstrap: 4.0.3

Bootstrap:

P.S.

I was searching for issues and I saw this one:
#2924

is it solved? why is it closed? I didn't see correct result in demos

[maxokorokov]

Hey, @Ardeshir81 !

Ok, I think we need an explanation here.
Looks like currently the positioning behaviour is a different in ng-bootstrap and bootstrap.

In ng-bootstrap when you specify <button ngbPopover position="right"> the popover will be positioned to the right no matter what (your case). Then it might become either squashed or overflow, so it overflows (we use css translate).

If you need popover to be positioned automatically, you should use just <button ngbPopover> or you might specify your preferences like

• <button ngbPopover [position]="['right', 'left']"> → try right, then try left, then fallback to right.
• <button ngbPopover [position]="['right', 'auto']"> → try right then fallback to auto that will go through the list of all available placements in this order.

P.S. I reopened the #2924 as a feature request, because it's a different kind of problem

[mendeza]

This is great to know. I wasn't aware of the use of isInViewport. The placement API doc does indeed state:

Placement of a popover accepts: "top", "top-left", [...] and array of above values.

However I didn't infer that viewport checking was at work. Perhaps the doc could be a bit more explicit? Or is it noted elsewhere, and I (and possibly others) missed it?

Regardless, this is a win for the OP's rtl scenario; ideally no need to have separate code for ltr and rtl either in his application code or in the ngbPopover component. I will test in my own rtl (Arabic) views.

Thank you!

[aghArdeshir]

Thanks @maxokorokov . yep! That does the work and does it actually very well. NICE!

I agree with @mendeza.

And nice job with the issues :)

BTW, I was struggling with the code. we are building a custom ng-bootstrap for our website, and I was assigned to a very specific use-case: ngbPopover on let, in RTL, in small screens (generally responsive mode) and I ended up with something like this:

As you see, The default is good, a little smaller screen is not bad, but the last scenario (smallest screen) is awful. It needs more processing and I'm not sure if it is the right solution.

I was trying to do ugly things like this:

\src\util\positioning.ts

BTW, I concluded that everything is fine and working and I was doing something that is already done in another way. I will talk to project manager and If he still insists on completing this, maybe I could come up with a nice standard solution and a nice PR.

Thanks!

I'm satisfied with the issue :)