Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Popover arrow placement can be incorrect when multiple placements are used #3040

Closed
reduckted opened this issue Feb 26, 2019 · 3 comments · Fixed by #3042
Closed

Popover arrow placement can be incorrect when multiple placements are used #3040

reduckted opened this issue Feb 26, 2019 · 3 comments · Fixed by #3042

Comments

@reduckted
Copy link
Contributor

Bug description:

When using multiple Placement values to position a popover, if none of them result in the popover being on screen, the first Placement is used to position the popover, but the last Placement is used to control where the popovers arrow appears.

image

Link to minimally-working StackBlitz that reproduces the issue:

https://stackblitz.com/edit/angular-ducwpd

Click on the button to open the popover. The popover will appear centered over the button, but the arrow will be to the right edge of the popover.

If you change the placement order in app.component.html to ['top', 'top-right', 'top-left'], the arrow will appear towards the left edge of the popover. If you change the placements to just contain top, the arrow is centered as you would expect.

Versions of Angular, ng-bootstrap and Bootstrap:

Angular: 7.2.6

ng-bootstrap: 4.0.4

Bootstrap: 4.3.1

@reduckted
Copy link
Contributor Author

I think this was introduced in either #3017 or #2972 when some of the positioning logic was changed. When it's trying the different placements, if the placement doesn't result in the element being on screen, it removes the CSS classes it added except if it's the last placement it's trying. If the final placement didn't put the element on screen, then it applies the first placement, but it still has the last placement's CSS classes applied.

// Remove the baseClasses for further calculation, except for the last one
if (baseClass && testPlacement !== lastPlacement) {
addedClasses.forEach((classname) => { classList.remove(classname); });
}
}

@fbasso
Copy link
Member

fbasso commented Feb 26, 2019

Thanks for your issue and investigation @reduckted !

Your're right, this last test testPlacement !== lastPlacement should't be here. It has been there after some different tries/refactor on positioning.

I'll prepare a PR soon.

@reduckted
Copy link
Contributor Author

Awesome! Thanks for the quick fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants