Skip to content
This repository was archived by the owner on Dec 5, 2024. It is now read-only.

Conversation

@Gudahtt
Copy link
Contributor

@Gudahtt Gudahtt commented Feb 9, 2018

Allows passing in a target prop to the Popper component directly, bypassing the need for the Manager and Target components. I implemented this feature for personal use initially, as it was needed in the development of a custom UI library.

The documentation and examples have been updated. I did try to make clear that the Manager and Target pattern is recommended and easier to use, and should be preferred in most cases.

It was challenging to think of a good example for this feature. I saw two options; get a ref and use that, or get an element directly from the DOM. I didn't want to do the latter, because it's generally recommended to avoid that when using React. But the ref was problematic as well. If I just grabbed a ref and used it right away, the resulting component would fail to pass in a target prop on the first render, because the ref doesn't get called until after the child component mounts. I considered briefly just using forceUpdate to trigger a second render in componentDidMount, but that was too confusing for an example.

Instead, I side-stepped the issue by writing an example that defaulted to not showing the popper component at all. That avoids the ref issue, because the popper is always hidden on first render, so it doesn't need to be shown. However, this gave me another concern - that people would look at the README for a quick example of how to make an open/close-able popover and grab that example without considering whether using Manager would suit their use case better. So to avoid that circumstance, I also added an example doing the same thing with Manager and Target first.

@FezVrasta
Copy link
Member

Does this support to pass an object as reference element?

@Gudahtt
Copy link
Contributor Author

Gudahtt commented Feb 9, 2018

This supports passing a DOM Element. That's the same thing a React ref callback would get you, or a call to document.querySelector or whatnot. It's the same thing that the Target component currently passes to the Manager, which passes it to Popper.

Is there another possible input that we should support? I hadn't thought of any other use cases.

@FezVrasta
Copy link
Member

@Gudahtt
Copy link
Contributor Author

Gudahtt commented Feb 9, 2018

Interesting; I didn't know about those.

I suspect the only thing stopping this change from being compatible with referenceObjects is the PropType validation. That's an easy change to make - I'll do that and test it this evening.

The Popper target previously was assumed to be made available via
context from the Manager component. Instead it can now be passed in
directly via props. This change required making the popover manager
in the context optional.

Closes #17
There doesn't seem to be any way to use PropTypes to validate that a
value is passed to a component via props *or* context. To make up for
this, an Error is now thrown in that situation.
@Gudahtt
Copy link
Contributor Author

Gudahtt commented Feb 9, 2018

The target prop now supports being passed a Popper referenceObject.

I also did some cleanup, mostly around lints and organization conventions that were different from when I originally branched.

@taras
Copy link

taras commented Feb 11, 2018

Allows passing in a target prop to the Popper component directly, bypassing the need for the Manager and Target components.

I'm glad I looked in Pull Requests because this is exactly what I needed. I was considering not using react-popper because reliance on context requires the target and popper to be rendered in the same context. In my case, I want to render the popper in a component that receives the target via a callback.

I considered briefly just using forceUpdate to trigger a second render in componentDidMount, but that was too confusing for an example. Instead, I side-stepped the issue by writing an example that defaulted to not showing the popper component at all. That avoids the ref issue, because the popper is always hidden on first render, so it doesn't need to be shown.

I'm using the same pattern in my application. Controlling rendering of the popper based on state makes popper more composable and flexible for more complex scenarios. 👍

@Justkant
Copy link
Contributor

Justkant commented Feb 22, 2018

I used to do the same because of some older version of react-portal that was making the use of the context impossible, not a problem anymore from my side.
It's a nice addition to the package

@flessard
Copy link

Any idea when you plan to merge this PR ?
Thanks :)

@ahx
Copy link

ahx commented Mar 15, 2018

I have tried this, but the position did not seem to change when the the target prop is changed.
Could someone confirm that it works for them when changing target dynamically (or confirm that it does not work for them as well)?

@Gudahtt
Copy link
Contributor Author

Gudahtt commented Mar 15, 2018

@ahx I think you're right. I'll look at fixing this now.

@Gudahtt
Copy link
Contributor Author

Gudahtt commented Mar 15, 2018

I believe it should update now when the target changes. Let me know if it works. Thanks.

@ahx
Copy link

ahx commented Mar 22, 2018

@Gudahtt It works as expected, thank you! react-popper is much easier to use for us now. I really like this PR. 👍

@FezVrasta FezVrasta merged commit 1bd24d8 into floating-ui:master Mar 22, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants