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

appendToBody option for Popover #852

Closed
spongessuck opened this issue Oct 7, 2016 · 6 comments
Closed

appendToBody option for Popover #852

spongessuck opened this issue Oct 7, 2016 · 6 comments

Comments

@spongessuck
Copy link
Contributor

spongessuck commented Oct 7, 2016

It looks like the positioning service is ready to accept an appendToBody parameter, but this option isn't included in the NgbPopoverConfig. I took a crack at editing the transpiled javascript in the npm module to see if it was possible and sort of got it working, but I'm wondering if there's a known issue standing in the way of it getting implemented.

@pkozlowski-opensource
Copy link
Member

Nope, there are no particular road-blocks, we just wanted to implement more generic version, see: #339.

We might just do a poor's man implementation for now that would just accept 'container="body"' only for now...

But yeh, the proper solution should come from #339

@spongessuck
Copy link
Contributor Author

Cool, I'll see if I can take a crack at it (I'm not sure how to write a test for it, though).

@pkozlowski-opensource
Copy link
Member

(I'm not sure how to write a test for it, though).

You can take a look at the existing tests for inspiration. Unfortunately we won't merge any PRs without tests. Ever. You have been warned :-) But at the same happy to offer some help for people struggling with tests.

@spongessuck
Copy link
Contributor Author

Unfortunately we won't merge any PRs without tests. Ever. You have been warned :-)

Known and understood! That's why I made the point.

@spongessuck
Copy link
Contributor Author

spongessuck commented Oct 7, 2016

I realize that what I'm asking about (appendToBody) isn't the same as what #339 wants to implement.

Is an appendToBody input not something you're looking for (I'm like 99% finished implementing it)? It was pretty easy since positioning was already set up for this use-case.

On that topic, I'm wondering if targeting non-browser environments will be a problem- right now my changes put the popover window in the root node (html).

  1. I'm using parentNode and appendChild to do this.
  2. I want to stop moving the popover window up the DOM tree when it gets 1 level down (body) so that there isn't a disconnect between what it's called and what it does.

Let me know your thoughts.

@spongessuck
Copy link
Contributor Author

spongessuck commented Oct 7, 2016

After looking at Positioning, it's using window and document to get the coordinates of the element in the page. If this isn't a problem regarding cross-platform, I think it'd be a good idea to rename everything to appendToRoot to be consistent since it's using documentElement for positioning, which in the browser is html, not body. It also means I don't have to change the component or test logic ;)

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

No branches or pull requests

2 participants