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

anchorEl prop for popover ? #2866

Closed
suhail-ansari-apconic opened this issue Jan 10, 2016 · 12 comments
Closed

anchorEl prop for popover ? #2866

suhail-ansari-apconic opened this issue Jan 10, 2016 · 12 comments

Comments

@suhail-ansari-apconic
Copy link

Passing it a ref to the component like Appbar as anchorEl does not work. Now i have not seen any documentation regarding what kind of value anchorEl needs. According to react documentation they discourage use of findDOMNode etc as it breaks encapsulation so refs should work.

@alitaheri
Copy link
Member

@suhail-ansari-apconic the El in the name stands for element. the usage of that function IS discourage, but the current implementation of popover requires it. you will need to pass in a DOM element for it to function. Also you will need to manage memory too (properly setting the anchor to null when closing) it could have been abstracted away but I wan't the one who implmented it so I don't know why it functions this way. There might be good reason. @chrismcv Any thoughts on this?

@chrismcv
Copy link
Contributor

Popover needs something to position itself to, which is always a DOMElement, as it calls DOMElement.getBoundingClientRect...
https://developer.mozilla.org/en-US/docs/Web/API/Element/getBoundingClientRect

In my view, it's easier for popover to take the DomElement an position itself, versus being passed a position directly. Because the element is being passed in, it will reside on state external to the popover itself, so popover can't manage that memory itself.

Perhaps popover could have an inline "auto anchor" element, e.g. an empty span, which renders wherever popover is declared in the markup. This might make the anchorEl redundant in some cases. Or alternatively, we could have anchorEl take a function a la react-bootstrap-overlays...
target={ props => findDOMNode(this.refs.target)}

@R4DIC4L
Copy link

R4DIC4L commented Jun 28, 2017

The current approach, with the DOMElement being provided for the popover to compute the position, breaks when wanting to provide a popover for a grid cell (popover declared outside the grid, with only one cell active).

Why? This is because the target element most probably needs to be saved in the state, which in turn triggers a redraw and the DOMElement no longer exists when given to the popover which will give a bounding rectangle with all properties set to 0. If, instead, ReactDOM.findDOMNode was called on the component (which was saved in the state on the click event on itself) in the render method, that would trigger a "findDOMNode was called on an unmounted component." error, because that component was re-rendered and the reference is old.

In such cases, one would prefer giving the bounding rectangle to the popover instead of the target element (and storing that instead of the DOMElement), as providing an old DOMElement will give bounding rectangle with all properties set to 0. If you think of any workaround for the above situation, please let me know.

@oliviertassinari
Copy link
Member

We have been porting the component on the v1-beta branch. We reimplemented it from the ground-up. While we haven't tested it, I think that the issue is most likely fixed on that branch. Hence, I'm closing it.
Still, we will accept PR fixes until v1-beta takes over the master branch.

@pre-martin
Copy link

It looks like Popover was moved to the package "internal", so I don't know, if it should be used any longer. But it still has the same properties as on the master branch (including anchorEl).

The new example for Simple Menus on https://material-ui-1dab0.firebaseapp.com/component-demos/menus also contains code with anchorEl. The value is retrieved (as for Popover) quite complicated via the event and stored in the internal state.

So I think that the original problem is still unsolved.

@oliviertassinari
Copy link
Member

@planetrenner-martin We plan on exposing the Popover component #6270. Noticed that it's rendering a backdrop on the page, for some situation you might want to use react-popper instead.

@bfang711
Copy link

Popover anchor to mouseevent.target winds up to pop over at upper-left corner of the screen. Any solution for that?

I know it works in v1 beta according to the website. However I'm still using material-ui v0.20.0. (seems like a lot of changes need to be made to migrate to v1-beta. for example, import {FlatButton} from '.../FlatButton'; needs to change to import {Button} from '.../Button';. If you know any better way of migration, pls let me know. I will try it as well. )

@Klynger
Copy link
Contributor

Klynger commented Mar 10, 2018

I'm having the same problem than you @bfang711, does anyone know a solution for that?

@mknet
Copy link
Contributor

mknet commented Apr 9, 2018

Hey guys, I got the impression that the positioning of the element does not work since the anchor element is re-rendered and is not yet placed into the real dom. That is why the Rect data is 0, 0, 0 and 0. :) I'm gonna try to avoid the re-rendering of the anchor element to check if this is the root cause.

Update 1:
At least the popover's position is correct if I use another element, in my case a table cell, in the same component. I'll keep exploring...

Update: 2:
My solution: Detect the rect of your anchor element on click, keep it in state and use the prop anchorPosition.

@R4DIC4L
Copy link

R4DIC4L commented Apr 9, 2018

Yes, but the property anchorPosition is only available in the v1.0 (next) material-ui implementation. Some of us are still using the v0.20 (master) implementation, which makes it difficult to use the Popover when re-rendering of the target element occurs and the element is not in the dom, making the rectangle (0, 0, 0, 0).

Maybe it would be nice to provide such a property, like anchorPosition, to manage the positioning rectangle from the outside of the component, to the master implementation as well?

@WolfyUK
Copy link

WolfyUK commented Jan 24, 2019

I'm not sure if my situation was exactly the same as above, but to resolve the (0, 0, 0, 0) position rendering I am setting style to the following instead of using anchorEl (TypeScript):

// ...get anchorElement from (e.g.) event.currentTarget or ReactDOM.findDOMNode()

const rect = anchorElement.getBoundingClientRect();
const style: React.CSSProperties = {
    transform: `translate(${rect.left}px, ${rect.top + rect.height}px)`,
};

<Popover style={style} open={this.state.open}>
    ...
</Popover>

@sherinshaju
Copy link

check this it will help you - https://codesandbox.io/s/use-ref-for-anchorel-forked-1pvh1

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