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

simplify the hooks api? #411

Open
ianstormtaylor opened this issue Mar 15, 2021 · 12 comments
Open

simplify the hooks api? #411

ianstormtaylor opened this issue Mar 15, 2021 · 12 comments

Comments

@ianstormtaylor
Copy link

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
Copy link
Member

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
Copy link
Author

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
Copy link
Contributor

@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
Copy link
Member

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
Copy link
Author

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
Copy link
Contributor

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
Copy link

patricklafrance commented May 19, 2021

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
Copy link

ulken commented Nov 3, 2021

@jchitel
Copy link

jchitel commented Nov 4, 2021

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
Copy link

@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
Copy link
Member

FezVrasta commented Dec 13, 2021

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
Copy link
Collaborator

atomiks commented Dec 13, 2021

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

8 participants