[ClickAwayListener] Expose a hook API

[Izhaki]

I needed ClickAwayListener, but a hook rather than a component.

So I went ahead and extracted it from this repo, and now considering a PR here. A hook will shrink the component code to this:

const ClickAwayListener = React.forwardRef(function ClickAwayListener(props, ref) {
  const { children } = props;
  const clickAwayRef = useClickAway(props, ref);
  const handleRef = useForkRef(children.ref, clickAwayRef);
  return <React.Fragment>{React.cloneElement(children, { ref: handleRef })}</React.Fragment>;
});

Before, a couple of questions:

document vs ownerDocument

Event subscription happens on document, whereas the contain check happens on ownerDocument.

I guess as no bugs were filed this works alright. But in more than a few DnD libraries out there subscription happens on ownerDocument (eg, to support iframe).

So why subscribe on document rather ownerDocument?

Why function and not arrow function?

As in with:

function mapEventPropToEvent(eventProp) {
  return eventProp.substring(2).toLowerCase();
}

Is there some convention in the code regarding when to use one or the other?

[oliviertassinari]

document vs ownerDocument

@Izhaki You are right, I have just tested it, it doesn't work, we need to use the document owner. At least, I had to make the change to have it work inside an iframe.

diff --git a/packages/material-ui/src/ClickAwayListener/ClickAwayListener.js b/packages/material-ui/src/ClickAwayListener/ClickAwayListener.js
index c689731b6..45ee7b22c 100644
--- a/packages/material-ui/src/ClickAwayListener/ClickAwayListener.js
+++ b/packages/material-ui/src/ClickAwayListener/ClickAwayListener.js
@@ -95,11 +97,13 @@ const ClickAwayListener = React.forwardRef(function ClickAwayListener(props, ref

   React.useEffect(() => {
     if (mouseEvent !== false) {
+      const doc = ownerDocument(nodeRef.current);
       const mappedMouseEvent = mapEventPropToEvent(mouseEvent);
-      document.addEventListener(mappedMouseEvent, handleClickAway);
+      doc.addEventListener(mappedMouseEvent, handleClickAway);

       return () => {
-        document.removeEventListener(mappedMouseEvent, handleClickAway);
+        doc.removeEventListener(mappedMouseEvent, handleClickAway);
       };
     }

Why function and not arrow function?

We use functions for the outermost scope.

I needed ClickAwayListener, but a hook rather than a component.

I'm curious about this, what's your use case for requiring a hook?

A hook will shrink the component code to this:

We very likely want the user to be able to push a ref (instead of pulling the value). This API can support it, which is great.

and now considering a PR here.

Awesome! :D

[Izhaki]

You are right

That's odd. Kind of implies not a single person out there used the component inside Cosmos v1... hard to believe.

And yes, that patch is exactly what I had in mind.

We use functions for the outermost scope.

Why?

I'm curious about this, what's your use case for requiring a hook?

A component forces me to compose it using HOC. Although I'm not 100% sure how this adventure will end up, you can get an idea of what's happening looking at this code - lots of opt-in features only two of which are HOCs (which I'm trying to phase out).

Some questions:

---

What's your preferred implementation with regards to keeping propTypes for these? Using a hook, props is simply passed to the hook.

---

Also - a stylistic choice alright - but what's your view on changing this:

const movedRef = React.useRef(false);
const nodeRef = React.useRef(null);

To:

const { current } = React.useRef({
  moved: false,
  node: null
});

---

Lastly, extracting a hook will not require changes to tests. But the iframe change will. Can you point me to the related tests and possibly an iframe example?

Perhaps this should be done in two PRs?

[oliviertassinari]

That's odd. Kind of implies not a single person out there used the component inside Cosmos v1... hard to believe.

What's Cosmos v1?
In my opinion "nobody" (a negotiable amount of people) renders in popups, iframes or shadow DOMs. For instance: Pomax/react-onclickoutside#236.

Why?

A pure convention.

lots of opt-in features only two of which are HOCs (which I'm trying to phase out).

Awesome, thanks for sharing, this composition approach of hooks looks interesting.

What's your preferred implementation with regards to keeping propTypes

We haven't been using prop-types for hooks so far. I think that you can keep them for the component. TypeScript can cover this problem, for people that value it.

but what's your view on changing this:

I'm not sure you can change it in this context, it seems that it would break useForkRef behavior
If we could, we tend to use one hook per concern in the rest of the codebase.

But the iframe change will. Can you point me to the related tests and possibly an iframe example?

I'm not aware we have any test for iframe, I don't think that we really need them, it's very niche.

[Izhaki]

Thanks. Will attempt a PR tonight.

[eps1lon]

We use functions for the outermost scope.

Why?

Better dev tools support most of the time and convention from other languages. Lambdas are usually used for anonymous/inline/"small" functions. A component or hook is neither one of these.

[Izhaki]

I'm going to wait with a PR for this until #18586 concludes.
There's a patch there that should probably go in before I extract the hook.

[oliviertassinari]

@Izhaki +1 for the preliminary iframe support patch :)

[Izhaki]

Sure. Any reason your patch above only covers mouse events (and not touch events):

  React.useEffect(() => {
    if (touchEvent !== false) {
      const mappedTouchEvent = mapEventPropToEvent(touchEvent);

      document.addEventListener(mappedTouchEvent, handleClickAway);
      document.addEventListener('touchmove', handleTouchMove);

      return () => {
        document.removeEventListener(mappedTouchEvent, handleClickAway);
        document.removeEventListener('touchmove', handleTouchMove);
      };
    }

[oliviertassinari]

@Izhaki This was only for brevity. It's very likely that we need the same for touch events.

[Izhaki]

Closing due to inactivity and as there much more valuable tasks in the backlog.

[oliviertassinari]

I have added the waiting for users upvotes tag. We are not sure enough people are looking for such capability. So please upvote this issue if you are. We will prioritize our effort based on the number of upvotes.