react-popper icon indicating copy to clipboard operation
react-popper copied to clipboard

simplify the hooks api?

Open ianstormtaylor opened this issue 3 years ago • 12 comments

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

ianstormtaylor avatar Mar 15 '21 00:03 ianstormtaylor

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

FezVrasta avatar Mar 15 '21 16:03 FezVrasta

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.

ianstormtaylor avatar Mar 16 '21 14:03 ianstormtaylor

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

dfernandez79 avatar Mar 31 '21 14:03 dfernandez79

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.

FezVrasta avatar Mar 31 '21 15:03 FezVrasta

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.

ianstormtaylor avatar Mar 31 '21 16:03 ianstormtaylor

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.

dfernandez79 avatar Apr 01 '21 17:04 dfernandez79

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.

patricklafrance avatar May 19 '21 14:05 patricklafrance

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

ulken avatar Nov 03 '21 15:11 ulken

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.

jchitel avatar Nov 04 '21 22:11 jchitel

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

chikunov avatar Dec 13 '21 10:12 chikunov

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

FezVrasta avatar Dec 13 '21 10:12 FezVrasta

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

atomiks avatar Dec 13 '21 11:12 atomiks