OnClickOutside for use with ES6 classes

[marnusw]

I'm using ES6 class React Components which means mixins aren't supported. I'd like to propose the following to enable such use. It uses the same implementation as the mixin except that the component is passed into the various functions rather than them being member functions of the component itself. This also lends itself toward some configuration when initially registering the component.

I'd like to add some documentation to the main readme etc., but I'm expecting some discussion around this and would like to confirm your interest in this solution before going there.

Excellent work on the mixin by the way!

[Pomax]

I don't think it's wise to solve the problem around ES6 classes and mixins in this repo, while React itself hasn't solved this problem yet. Instead, I'd like the put up for consideration that it is more important that you file this solution on the React repositoriy on http://github.com/facebook/react/issues, so that they can have a look at what you're doing and whether that's a solution that they want to adopt as official "and here is how you do all this using ES6/ES2015". If there is consensus that this is a way forward for React, then I will be more than happy to accept PRs that implement the official solution.

I'll keep this PR open for now, because if they accept your solution then we can pretty much immediately land it, but I won't land it quite yet.

[alexkuz]

@marnusw Why not just make a higher-order component for that, that provides onClickOutside() and other functions in props?

[marnusw]

Thanks @alexkuz, based on your suggestion I have updated the solution with a higher-order component/wrapper function. This is much simpler, and I believe closer to the current de facto standard way of extending ES6 class components. I'll take it to the React team for comment from here if there is no further input.

[Pomax]

Nice! Go for it.

I'm not a fan on the notation myself, since it is an obvious solution on first thought, but further thought reveals rather insane notation if you carry it over to complex real world UIs:

<ThingWrap>
  <ExtensionWrapping>
    <AuthFunctionality>
      <SecondaryUI>
        <OutsideClicks>
          <VRSupport>
            <div>Oh my god why</div>
          </VRSupport>
        </OutsideClicks>
      </SecondaryUI>
    </AuthFunctionality>
  </ExtensionWrapping>
</ThingWrap>

I still need to read through ECAMScript 262 v6 to see if there are neat things that ES6 allows that we haven't started taking advantage of yet...

[marnusw]

As I understand it the idea is that you build up the functionality of a specific component with wrappers within a single file, and then still use a single component in the parent; i.e. the wrapping is transparent outside the inner component's definition, consider:

// FocussedSection.jsx

class FocussedSection extends React.Component {
  handleClickOutside = (event) => {
    // Remove Focus
  }
}

export default listenToClickOutside(FocussedSection);

// SectionList.jsx

import FocussedSection from './FocussedSection.jsx'

class SectionList extends React.Component {
  render() {
    return (
      <div>
        {sections.map(section => <FocussedSection {...section}/>)}
      </div>
    );
  }
}

The outcome is exactly the same as applying the mixin to the FocussedSection component as far as I can see, and your example above would become:

<ThingWrap>
  <ExtensionWrapping>
    <AuthFunctionality>
      // No OutsideClicks or VRSupport components here if they are both wrappers
      // inside the SecondaryUI file.
      <SecondaryUI>Not so bad</SecondaryUI>
    </AuthFunctionality>
  </ExtensionWrapping>
</ThingWrap>

Unless I'm missing something which is very possible...

The idea stems from this blog post originally and is also used by:

• Yahoo in Fluxbile: See connectToStores() wrapper, which also supports the ES7 decorator pattern.
• Material UI in their handling of themes (see section 2. about half way down), where they also use a decorator.
• I haven't checked behind the scenes, but Relay uses a similar syntax. Look at the last code block in this example.

I think an ES7 decorator might well be the "neat thing to take advantage of" here. I could add decorator support to this PR as well!

[mwx-limited]

@Pomax, given the most recent comments on facebook/react#5010, what are your thoughts?

If you'd be willing to merge in principle I'll get this PR ready for merge. I can hardly remember the status of it, but I'm sure it still needs some docs etc.

[Pomax]

In principle it's fine, but it completely changes how this thing works, so it'll basically be a deprecation of everything currently in the repo, with a new major version, new docs, and new code, since it won't be a mixin anymore.

[marnusw]

I was thinking the two options could run in parallel? That is why I was glad that I eventually found a solution which is a minimal wrapper around the mixin allowing use as a decorator with a similar interface, i.e. you still only have to declare the handleClickOutside method, and apply either the mixin or decorator depending on preference.

Let me see if I can update the docs etc. in the next few days so it reflects the two options, then I'll check back to see what you think. Unless you definitely want to deprecate the mixin from your side. I think mixins are still very much in common use though.

[Pomax]

looking forward to that. One thing that might happen is that this repo gets deprecated in favour of react-onoutsideevent that came out of #45, but for the while that project has not been bower/npm published yet.

[marnusw]

I've updated the documentation to indicate how the higher-order component/decorator might complement the existing mixin rather than replace it. I would love to hear your thoughts.

You might note that I added one extra feature in the decorator which is as yet undocumented: the ability to pass a handler down on the onClickOutside prop from the parent rather than placing the handler in the child. I've found this useful on many occasions where the logic, for example, for showing a menu is maintained in a parent, but clicks outside the menu component should be detected. I'd like to leave that in, documented or undocumented, depending on your opinion.

I looked over react-onoutsideevent briefly and it looks like the API is quite similar although the implementation might be different. (I didn't look at the code.) That being the case these changes I propose can also be ported to that library once it has stabilized, and I would be happy to do so if you'll let me know when the time is right.

As a side note, I like the camel casing of the disableOnClickOutside={true} prop in this version since it feels more React-like with them having converted all events/attributes to camel cased versions. The new disableonoutsideevent={true} would be less so.

[Pomax]

The outsideevent project (which is basically this mixin, but made generic) does use listenToOutsideEvent and disableOnOutsideEvent though.

[Pomax]

hm, I'm not sure a .gitignore is necessary for these files - it's easy enough to pick them out of a PR.

[Pomax]

If you can take out the gitignore then this looks good enough to merge to me - it might be worth filing a similar PR against the new generic outsideevent project; it will probably replace this mixin as recommended way to deal with outside clicks, but then "or other event types" rather than not having the option which event(s) count as outside-listenable.

[marnusw]

Thanks @Pomax, the .gitignore is gone, I had only added it for convenience. This is good to merge then.

I'll put filing a PR on the outsideevent project on my to-do list. I might do so next weekend.

[Pomax]

@marnusw: thanks! One last q, would it be possible to squash this to one commit? (just to make reverts easier should they end up being necessary)

[marnusw]

Sure, I can do that. As far as I know if you do a merge via GitHub or using git merge --no-ff, so you get a new merge commit in master, removing that merge commit will pluck out all the commits from the feature branch as well; so squashing features to cater for reverts isn't strictly necessary. But I'll squash this for you in bit!

[marnusw]

Squashed, ready to merge...

[Pomax]

awesome, thanks!