13

I would like to avoid manually adding the onClick property to my custom components.

For that, I thought about a Higher Order Component named WithClick that would wrap my components with the integrated onClick property.

The problem I'm facing is that in order to wrap it, I have to use an additional <div /> tag to access the event property. And this tag is messing up my CSS.

Example : 

import React, { Component } from 'react'

const WithClick = (WrappedComponent) => {
  class BaseComponent extends Component {
    render () {
      return (
        <div onClick={this.props.onClick}>
          <WrappedComponent {...this.props} />
        </div>
      )
    }
  }

  return BaseComponent
}

export default WithClick

The solution would be a hack, allowing to attach an onClick event to a <React.Fragment /> tag or something similar.

I tried attaching ref to the child, but it doesn't work, I have to treat the ref prop in the child so there's no point doing that.

Do you know a workaround ?

Improve this question
7
  • Can I ask why you want to avoid adding onClick? Commented Feb 4, 2019 at 17:33
  • 1
    I am curious what WrappedComponent looks like as I am unsure why you do not want to pass onClick down to it. You still want the user to click a visible object don't you? Whether it's a thumbnail, a button, etc and those things have a layout at the end of the day. Commented Feb 4, 2019 at 17:35
  • @georgeperry because it generates a lot of redundant code. Commented Feb 4, 2019 at 17:40
  • I have to agree with @BenHerbert, it doesn't seem sensible. the unnecessary code it generates in the DOM is relatives minute compared to the extra code you would need to write to get around this Commented Feb 4, 2019 at 17:43
  • @georgeperry The question is all about improving the code. Not about "Is it important to lost few seconds ?". Commented Feb 4, 2019 at 17:47

4 Answers 4

Reset to default
9

While I'm also a little skeptical of the need for this, I could see some specific case where you are adding the same handler to a lot of components and it would be necessary.

ReactDOM.findDOMNode is discouraged by the docs, and deprecated in strict mode. React.cloneElement is a better option. 

const addClickToComponent = ({component}) => (
  React.cloneElement(component, {
    onClick: someComplicatedClickFunction,
  }
);

const ComponentWithClick = addClickToComponent(noClickComponent);

I usually make these wrapper components rather than HOC, and if you need to add a property to multiple children, you can do that.

const ClickWrapper = ({children}) => (
  <React.Fragment>
    {React.Children.map(children, child => (
      React.cloneElement(child, {
       onClick: someComplicatedClickFunction,
      )
     )
    }
  </React.Fragment>
 );

 // jsx

 <ClickWrapper>
   <ChildComponent />
   <ChildComponent />
   <ChildComponent />
 </ClickWrapper>
Improve this answer
Sign up to request clarification or add additional context in comments.

3 Comments

This is exactly what I was looking for. My example with onClick wasn't really good, but you are right, there are similar cases. Thanks.
I use this often for style helper components for the UI library I maintain. for instance a <Spacing> component that adds appropriate margins for layout and passes an emotion.js style to wrapped components, so there are definitely use cases. I could see the onClick case being useful if you were adding a hander that had a lot of boilerplate or something, it could neaten up your code a lot. Good luck!
Thank god for people who stick with the question in the title instead of going "don't do A, do B" route. For the OP, it might actually be better to do it differently but for many people who get here from google it is not 😊. Thanks.
2

Wrapper Component

class Wrapper extends Component {
    render() { 
        return ( 
        <React.Fragment>
            <WrappedComponent />
        </React.Fragment>);
    }
    componentDidMount(){
        //This will return the container of WrappedComponent
        ReactDOM.findDOMNode(this).onclick = this.props.onClick
    }
}
export default Wrapper;

Wrapped component

class WrappedComponent extends Component {
  render(){
    return(
      <div className="wrappedComp"><span>I am wrapepd component</span></div>
    )
  }
}
Improve this answer

2 Comments

It works. but as @JustH mentionned, ReactDom.findDOMNode() is deprecated.
@Rémy Machado I think i missed it.
2

In some cases it may be handy to use display: contents; on a wrapper element, so additional wrapper with onClick will not break you code.

Improve this answer

Comments

1

for those who don't want to mess up the CSS, using a span did the work for me.

Improve this answer

1 Comment

Not a solution. "Span not messing up CSS" is not a rule but a specific. It works for you, but will not work for other, who need block element for example.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.