floating-ui / react-popper

🍿⚛Official React library to use Popper, the positioning library
https://popper.js.org/react-popper/
MIT License
2.5k stars 226 forks source link

simplify the hooks api? #411

Open ianstormtaylor opened 3 years ago

ianstormtaylor commented 3 years ago

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:

import React, { useState } from 'react';
import { usePopper } from 'react-popper';

const Example = () => {
  const [referenceElement, setReferenceElement] = useState(null);
  const [popperElement, setPopperElement] = useState(null);
  const [arrowElement, setArrowElement] = useState(null);
  const { styles, attributes } = usePopper(referenceElement, popperElement, {
    modifiers: [{ name: 'arrow', options: { element: arrowElement } }],
  });

  return (
    <>
      <button type="button" ref={setReferenceElement}>
        Reference element
      </button>
      <div ref={setPopperElement} style={styles.popper} {...attributes.popper}>
        Popper element
        <div ref={setArrowElement} style={styles.arrow} />
      </div>
    </>
  );
};

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:

import React from 'react';
import { usePopper } from 'react-popper';

const Example = () => {
  const { targetRef, popperRef, arrowRef } = usePopper({
    modifiers: [{ name: 'arrow' }],
  });

  return (
    <>
      <button type="button" ref={targetRef}>
        Reference element
      </button>
      <div ref={popperRef}>
        Popper element
        <div ref={arrowRef} />
      </div>
    </>
  );
};

If you get rid of the arrow in the simplest case it becomes even nicer:

import React from 'react';
import { usePopper } from 'react-popper';

const Example = () => {
  const { targetRef, popperRef } = usePopper();
  return (
    <>
      <button type="button" ref={targetRef}>
        Reference element
      </button>
      <div ref={popperRef}>
        Popper element
      </div>
    </>
  );
};

The targetRef and popperRef values would be functions that the usePopper 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.)

FezVrasta commented 3 years ago

The reason why the refs management is left to the consumer is because refs could be reused by other parts of the consumer code.

ianstormtaylor commented 3 years ago

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.

dfernandez79 commented 3 years ago

@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 setState combined with ref, you also need to use something similar to merge refs (to combine the refs with a ref callback):

// 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.

FezVrasta commented 3 years ago

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.

ianstormtaylor commented 3 years ago

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.

dfernandez79 commented 3 years ago

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.

patricklafrance commented 3 years ago

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:

    const updateStateModifier = React.useMemo(
        () => ({
            name: "updateState",
            enabled: true,
            phase: "write",
            fn: ({ state }) => {
                const elements = Object.keys(state.elements);

                setState({
                    styles: fromEntries(
                        elements.map(element => [element, state.styles[element] || {}])
                    ),
                    attributes: fromEntries(
                        elements.map(element => [element, state.attributes[element]])
                    )
                });
            },
            requires: ["computeStyles"]
        }),
        []
    );

UPDATE:

Turns out I don't need to provide an updateStateModifer since the applyStyle modifier is already applying the styles and attributes on the reference element.

Except for data-popper-reference-hidden and data-popper-escaped attributes though but I am not sure what they are used for.

ulken commented 2 years ago

How Headless UI does it: https://github.com/tailwindlabs/headlessui/blob/main/packages/@headlessui-react/playground-utils/hooks/use-popper.ts

jchitel commented 2 years ago

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 createPopper, and it's a hook, but that doesn't necessarily make it idiomatic React.

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 usePopper:

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 usePopper, where this component could be for simple cases where all you need is declarative HTML as the reference and popper, and usePopper would be used in cases where you actually need fine control over the DOM.

chikunov commented 2 years ago

@jchitel This wrapper API creates an implicit dependency on the first node in children/popper to be an html node/ref forwarding component. Which becomes even more awkward if popper content/reference are components on their own, which is most of the time in my practice.

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.

FezVrasta commented 2 years ago

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

atomiks commented 2 years ago

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