-
-
Notifications
You must be signed in to change notification settings - Fork 226
simplify the hooks api? #411
Comments
The reason why the refs management is left to the consumer is because refs could be reused by other parts of the consumer code. |
I think people could use something like https://github.com/gregberge/react-merge-refs if they need to use multiple refs in the same component. But it’s probably not the most common case. |
@FezVrasta I know that API breaking changes are usually not welcome by users, but I'll love to see the changes proposed by @ianstormtaylor When you use // pseudo-code like.. some vars are undefined, also
// assume that you have a useCombinedRef to combine the refs
const ComponentWithRef = (props, ref) => {
const [referenceElement, setReferenceElement] = React.useState(null);
const [popperElement, setPopperElement] = React.useState(null);
const { styles, attributes } = usePopper(referenceElement, popperElement, {
placement,
strategy,
modifiers,
});
// You'll need to combine the ref & setReferenceElement
const combinedRef = useCombinedRefs(ref, setReferenceElement);
return <><div ref={combinedRef}><div ref={setPopperElement}>Pop over</div></>
} So the current API has the same ref management issues. |
How would @ianstormtaylor proposal solve this? You wouldn't be able to use the Popper refs on other logic that expects "classic" refs, since Popper uses ref callbacks. |
I believe @dfernandez79's point is that the current API already has ref-combining issues, so making this change wouldn't actually add a new class of issue to deal with (since it already exists)—so it's just one more point in favor of the simpler API. |
Yes, that was the intention of my comment. |
Does anyone have develop such an API using ref instead of accepting elements as args? I am trying to write my own but have issues replicating the following code, it loop indefinitly when I update the state:
UPDATE: Turns out I don't need to provide an Except for |
I'm wondering why we even need to bother with direct DOM references at all. I get that Popper itself is built for vanilla JS, but abstractions should generally try to be as idiomatic to the framework as possible. This API seems to be a basic wrapper around I believe that the idiomatic approach to this API would be a component that serves as a container for the reference content and allows the popper content to be passed as an alternative prop. Here's a very simple implementation as a wrapper over function Popper({ children, popper, options }) {
const [referenceElement, setReferenceElement] = useState(null);
const [popperElement, setPopperElement] = useState(null);
const { style, attributes } = usePopper(referenceElement, popperElement, options);
return (
<>
<div ref={setReferenceElement}>{children}</div>
<div ref={setPopperElement} {...attributes.popper} style={style.popper}>{popper}</div>
</>
);
}
function MyComponent() {
return (
<Popper popper={<div>popper</div>} options={...}>
<div>reference</div>
</Popper>
);
} This could be provided in addition to |
@jchitel Existing API is an idiomatic React approach where this hook provides a chunk of reusable logic and all presentation is up to the user. This is a lot more flexible than providing components. |
We are working on a new library to work with React and Floating UI (the new version of Popper), you can join the development on https://github.com/floating-ui/floating-ui |
Floating UI's hook takes the approach @ulken linked: https://floating-ui.com/docs/react-dom There will be an extra hook for behavioral stuff to craft popovers, tooltips, etc. more easily than just positioning |
I'm trying to use
react-popper
but really confused by the docs and why the API is so complex. It forces the end user to keep track of lots of interim state. The current example:Seems like it could be changed to return the refs that the user should attach instead, and keep track of all the internal state and styles itself. That would simplify it to:
If you get rid of the arrow in the simplest case it becomes even nicer:
The
targetRef
andpopperRef
values would be functions that theusePopper
returns, and whenever they get called it can sync its internal state.Maybe I'm missing something?
The only "downside" is not be able to spread the
styles/attributes
props yourself, but honestly that sounds like an upside to me. (I believe you could actually design the API to allow spreading those props still if you really wanted to.)The text was updated successfully, but these errors were encountered: