Add click-outside hook

[nathanpower]

Let me know thoughts on this? No internal state, I figure most uses will be to simply close/hide the target component, so callback probably sufficient. Open to being convinced otherwise though.

[lpww]

Thanks @nathanpower ! This is working well. We removed callbacks from our other hooks as the returned state was already being updated. I think for this case it does make sense because, as you say, there is no state required. @jh3y what do you think?

[cianfoley-nearform]

In the docs I think its best to call the hook Click Outside Hook, we changed this on other docs.

https://react-browser-hooks.netlify.com/?selectedKind=PageVisibility&selectedStory=Default&full=0&addons=1&stories=1&panelRight=1&addonPanel=REACT_STORYBOOK%2Freadme%2Fpanel

[cianfoley-nearform]

same here

[cianfoley-nearform]

Looks good to me Nathan! Nice work.

[jh3y]

I'd maybe be a little less restrictive on the el parameter here.

An array of nodes might be more powerful. Accepting either single or an array. I'd then make the checks a little more thorough. Making use of .contains would be ideal 👍

This isn't optimal but something along these lines potentially. The reason for this being you might have a layout component whose children are the main content of the app. An outside click would never fire in this case. If you can pass an Array of refs, you can avoid this issue.

const handler = (ev) => {
  const target = ev.target
  let outside = false
  if (el.current.length) {
    for (const node of el.current) {
      if (!node.contains(target)) outside = true
    }
  } else if (!el.contains(target)) {
    outside = true
  }
  if (outside) onClick(ev)
}

[nathanpower]

you might have a layout component whose children are the main content of the app. An outside click would never fire in this case.

Just trying to understand this use case... what is ev.target in this scenario, and what is el?

[jh3y]

For example, we have projects where the router instance is wrapped in a layout component that compromises of the app content. You'd likely use an "off" click here for a burger menu style component. But everything on the page is a child of the element. And parentElement won't detect all scenarios. It's purely to be more flexible as people will build things in all different ways.

One example could be that you have a header and side menu. When it's open, you want to close it on "off" click. But you don't want it to close when you click the header or side menu or any descendant of each.

That way I could have something like useClickOutside([header.current, sidemenu.current], handleClickOutside)

[nathanpower]

Thanks, this makes sense. We solve this issue in a current project by having our ClickElsewhere component take an ignore array of class-names. We also ignore anything that has a data-click-elsewhere-ignore attribute. I was going to follow up with another PR that added something like this, but I think your suggestion is better.

[nathanpower]

@jh3y does a565ab2 improve matters for you?

This solves the case where we want to not call onClick when clicking on certain components.

Your earlier comments seem to also suggest another case where onClick is not called where we do want it to be, if so I'm still not sure I understand this case (sorry).

[jh3y]

@lpww 👍

A callback makes sense here for sure. It's more declarative. I think the checks/params could be a little more powerful. I've left a comment about this.

[nathanpower]

Thanks for the feedback, will update

[nathanpower]

Possibly makes sense to not merge this until #105 is in, and I'll add the checks for SSR

[nathanpower]

😆

[jh3y]

Maybe add the window.removeEventListener('click', handler) inside here also to clean it up on off click but not necessarily unmount 👍

[nathanpower]

Won't this remove the handler for that component on the first off-click? I think we want to keep it there until it's unmounted.

I guess we do need to check each ref exists though, as each may be unmounted, and the cleanup handler of useEffect might not be called it may not be a ref to the component that is consuming the hook. (If that makes sense).

I guess the handler should also be removed if all els are falsey.

[nathanpower]

I guess the handler should also be removed if all els are falsey.

Actually, on second thoughts, probably shouldn't do that. There is nothing stopping a component passing an array of refs to components other than itself, and this component will expect the handler to stay around for it's own lifetime, even if all the targets are unmounted and remounted again. I'll go with the truthy check for now.

It's a bit of an edge case I guess, I imagine most usage will be the component passing its own ref.

[jh3y]

It will remove it you're right. And that's because the effect only runs when el changes.
This does bring up something I think we've overlooked though 🤦‍♂️
You don't want to bind the handler on every render. Because that means the off click callback is firing even when it doesn't need to.
Think of the case where you have a menu that slides in and out. You don't want the handler firing every time the user clicks the page even if the menu is closed. So maybe it requires a parameter that tells it if it can bind, then we can conditionally bind the event in the useEffect function.

[nathanpower]

You don't want to bind the handler on every render

I thought that it bound only when el changed, (as well as mount/unmount) as el is passed in the array of dependencies (last argument to useEffect). Am I wrong there?

In any case, I think we only want to bind the handler on mount, and remove on unmount, so I think I can pass an empty array as last argument to achieve that.

Passing in an empty array [] of inputs tells React that your effect doesn’t depend on any values from the component, so that effect would run only on mount and clean up on unmount; it won’t run on updates. - https://reactjs.org/docs/hooks-reference.html#useeffect

[jh3y]

Yes. Sorry I probably didn't word that right.

Currently, that handler will fire on every click as soon as the component is mounted. You don't want that.

You only want to bind when some condition is met. For example, a menu in an open state.

[nathanpower]

That's an interesting one... should be enforce that this condition be supplied, or would you be OK with opt-in, like 09b7b01?

Here we are back to running the hook on every render, but adding/removing the listener depending on an optional predicate.

You would use this like e.g useClickOutside(ref, optionsVisible, hideOptions)

[jh3y]

Yeah I think that makes sense 👍

[lpww]

@nathanpower the ssr pr has been merged. @jackdclark had a sample app that he was using to test the other hooks on a server. you may be able to get a hold of his demo?

[nathanpower]

If I understand correctly, because I am only referencing window inside useEffect, there is nothing to do here with regards to SSR?

Let me know if you want me to squash the commits, there are quite a few now.

[lpww]

Oh yes, good point. I think you're right that it should work with SSR! Any chance we can make the boolean into an options object? We've followed that pattern in some of our other hooks so I think it makes sense to keep that consistency here.