[Core] Remove click-awayable mixin

[newoga]

Related to #2437

Let me know if we should deprecate instead of remove.

Done:

• Removed from Snackbar
• Removed from Menu
• Removed from TableBody
• Remove mixins/click-awayable.js and references in index.js

Outstanding:

• Fix validation error when rendering <noscript/> in <tbody/>

[oliviertassinari]

I like this solution 👍. I can't see better way to do it.

• I have founds only one repository that implement a clickaway feature: https://github.com/javierbyte/react-clickaway. They are using our mixin 😁.
• Regarding the manuallyBindClickAway, that's only used by the Snackbar. We can implement the logic inside the component or use.
• I'm wondering why RenderToLayer use the touchstart and the click events for the click away.

[newoga]

I like this solution 👍. I can't see better way to do it.

Sounds good, I'll continue to pursue this.

Regarding the manuallyBindClickAway, that's only used by the Snackbar. We can implement the logic inside the component or use.

What do you think is the best way to do this? Add a delay prop and let the ClickAwayListener manage the timeout? Or do we need to find a more sophisticated way to let parent component managing binding.

I'm wondering why RenderToLayer use the touchstart and the click events for the click away.

I wondered the same thing. My initial reaction was that RenderToLayer was reimplementing clickaway functionality without the mixin. Any other thoughts?

[oliviertassinari]

What do you think is the best way to do this?

I think that it's simpler to put the logic inside the Snackbar, I don't think that adding a delay property will be enough.

Indeed, RenderToLayer is reimplementing the clickAway logic. But this component isn't following the classical React lifecycle. I don't think that we can do anything other than using the right event name.

[newoga]

• Implemented ClickAwayListener for TableBody and Snackbar

[newoga]

Still need to remove this...

[oliviertassinari]

I guess we need to add the PropTypes

[newoga]

@oliviertassinari @alitaheri This is almost done. The only way I can currently think of fixing the tbody issue is to not use react-event-listener and just manually bind the events in ClickAwayListener, that way nothing is rendered in the DOM.

The other approach is if react-event-listener just rendered it's children the same way ClickAwayListener does, or if there was a way for it to attach it self at the end of body, that might fix it.

[newoga]

@oliviertassinari

Indeed, RenderToLayer is reimplementing the clickAway logic. But this component isn't following the classical React lifecycle. I don't think that we can do anything other than using the right event name.

I think the mouseUp and touchend events used in the mixin seem more appropriate than the click and touchstart events used in RenderToLayer. Do you agree? Should we switch them in this PR?

[oliviertassinari]

@newoga I was thinking the same. Until I realized that http://jedwatson.github.io/react-select/ is using mousedown and touchstart.
I agree with them. For instance, have a look at the native <select /> behavior.

[alitaheri]

@newoga

manually bind the events in ClickAwayListener

I think this is indeed better 👍

[oliviertassinari]

What about rendering the children before the listener?

[oliviertassinari]

@newoga That looks good so far. We are close to merging this 👍.

[newoga]

Sounds good, I'm going to take another stab at this tonight and hopefully we'll be able to finish it. After this, there is only one component that is using the style-resizable mixin we'll need to take care of. 😄

[newoga]

@alitaheri @oliviertassinari Can you look at this revised implementation? I couldn't see a way around the <tbody /> so I implemented the event handlers manually. I tested this across all the components that were using the mixin and it looks like it's working well.

Thanks!

[alitaheri]

How about rendering children directly? const el = ReactDOM.findDOMNode(this); would work anyway. This component no longer needs to render anything extra, it will be like a wrapper that finds it's first node and attached the click away to it. I think it's best to remove type:

return children;

And then render any dom element that we wish to attach it to as the child?

This is not tested, I don't know if it will work or not. just thinking out load 😅

[newoga]

Mm, that might work (not before though when we were using the react-event-listener). Though we'd have to go through each of the components to add the appropriate type as a child of the ClickAwayListener.

[alitaheri]

If don't have the time I can take it from here. I have a lot of time today 😁 😎

I think it's time you take some rest anyway O.o You've done great 👍

[newoga]

One sec, I'm trying it now. If it needs more fixes after feel free to pursue! Thanks!

[alitaheri]

Sure, Thanks a lot 👍 I'll take it from where ever you leave off 😄

[newoga]

@alitaheri Returning this.props.children works (after fixing the components to have the root dom node as a child rather than inline). I also tried to make the timer code snackbar clearer. Feel free take it from here if you want any more changes and you and @oliviertassinari can get this merged. Okay 😴 time.

Thanks @alitaheri! 😁

[alitaheri]

Well I didn't expect you do it all O.O Thank yo so much @newoga This is beautiful 😍 The implementation is awesome 👍

All green here 👍 😄

@oliviertassinari Feel free to merge if all's good.

[oliviertassinari]

@newoga Really nice work 👍.