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

Should we support explicit targets? Should we allow selector strings to be passed in for the target? #109

Open
thiberaw opened this issue Jan 23, 2018 · 6 comments

Comments

@thiberaw
Copy link

In attach-popover.js file there's a _initializeAttacher method triggered on insert and by an observer on 'hideOn', 'showOn', 'popperTarget' properties.

In the hbs file where the attach-popover is declared if you pass a "#myId" string to the popperTarget attribute, then in the _initializeAttacher this property is set like this:

this._currentTarget = this.get('popperTarget')

but because the popperTarget is a string, then the _currentTarget is no longer a DOM element, so when we run the:

this._currentTarget.addEventListener(event, this._showAfterDelay)

an error is raised because this._currentTarget has no addEventListener ( of course, it is just a string :)

I came to some workaround and passed a DOM element to the popperTarget in the hbs file, but I think it would be better to do as in your ember-popper add-on and check if the this.get('popperTarget') is an instance of Element (cf. the _getPopperTarget method in the ember-popper-base.js file)

If you want, and think it worths it, I would be pleased to send you a pull request for this issue.

@kybishop
Copy link
Collaborator

kybishop commented Jan 26, 2018

Hey @thiberaw, I have concerns about specifying the target instead of finding it automatically:

  1. What happens to the attachment when/if the target is destroyed? Possible memory leak here.
  2. What circumstances require an explicit target instead of an automatic target on the component's parent? (e.g. are we adding extra complexity to the addon when we might not need it?)

I haven't had a chance to reach out to the community about this yet, so I've left the current stop-gap that you found: for now you can pass in a DOM element as the target. I'd like to come to a consensus on how we should handle explicit targets (assuming we should handle them at all!)

@thiberaw and others, anyone have thoughts on explicit targets?

  • Are they actually needed?
  • Should we allow selectors to be passed in?
  • What do we do if an explicit target is destroyed? Just leave it up to the user to handle this case?

@kybishop kybishop changed the title check if popperTarget si a DOM element Should we support explicit targets? Should we allow selector strings to be passed in for the target? Jan 26, 2018
@billdami
Copy link

@kybishop as an extension of this, is there a path currently available (or planned) that would allow a tooltip to target arbitrary coordinates on the page, instead of a specific DOM element?

An example use case for this (which I currently have), is showing tooltips for objects within a <canvas> element. In this case there obviously is no DOM element to target, but the page coordinates of an object rendered in the canvas can be determined.

@mriska
Copy link

mriska commented Jun 19, 2018

I found it useful to be able to provide an explicit target when using the tooltip as a contextual component for my own components.

I added it like this in my own component:

{{yield (hash popover=(component 'attach-tooltip' popperTarget=element)) }}

And I was then able to have contextual tooltips for my components like this:

{{#my-component as |task|}}
  Something that I wish to yield in my component
  {{#task.popover}}
    Here be popover
  {{/task.popover}}
{{/my-component}}

@ctjhoa
Copy link
Contributor

ctjhoa commented Jun 20, 2018

@billdami I think you could create a dummy div

<div id="myDiv" style="position:fixed; left: {{position.x}}; top: {{position.y}}"></div>

And then create an attacher with #myDiv has popperTarget

@thiberaw
Copy link
Author

@kybishop
first: sorry for my late answer ...
second: I do understand all of your concerns

@mriska
this solution, if I do understand clearly, is the workaround I came with, but what I proposed was using the _getPopperTarget method to get the DOM element in a more simple way

@mriska
Copy link

mriska commented Jun 20, 2018

@thiberaw, agreed, I see the ability to provide an explicit target as essential for the kind of scenario I demonstrated earlier (yielding a contextual popover component) and it would be sad to loose this possibility. I don't think providing the element directly, as in my example, is a workaround. It is my belief that it is how popperTarget is supposed to be used in explicit target scenarios. '

@kybishop, I believe there is an actual need for explicit targets. Whether this is done by selector or by passing the element doesn't matter that much to me personally. With regards to destruction it would be good if it can be handled automatically, less possibility of doing something wrong for the developer.

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

No branches or pull requests

5 participants