Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Inherited prop className #4341

Closed
mjurczyk opened this issue Jul 10, 2015 · 2 comments
Closed

Inherited prop className #4341

mjurczyk opened this issue Jul 10, 2015 · 2 comments

Comments

@mjurczyk
Copy link

Hey,

we have been recently using React (+Flux) in our project and we approached a quite intriguing problem. Following the WebComponents-like approach, we try to create a bunch of reusable components with similar behaviours. The problem arises when we want to put the same component, with the same 'logic', on a different view, with a different design (that be -> a different CSS class attached). We need to create the same component with different set of class' names (+usually a few default ones) and right now it seems to be only possible by passing props like classes or inheritedCls.

_tl;dr;_ It would be really cool if React passed the className from the component definition to it's actual DOM elements. That would save us creating and passing a classes props, ex.:

_Initialization in a parent component_:

class ParentComponentAlpha {
    render () {
      return (<MyComponent className={'contextual-styling-1'} />);
    }
};

class ParentComponentBeta {
    render () {
      return (<MyComponent className={'contextual-styling-2'} />);
    }
};

_Current way of appending 'inherited' classes_:

class MyComponent {
  propTypes: {
    classes: React.PropTypes.arrayOf(React.propTypes.string)
  },

  render () {
    var innerClasses = [
      'cmp--default',
      'cmp--mobile'
    ];  

    // Renders <div role="my-cmp" class="cmp--default cmp--mobile contextual-styling-."> ... </div>
    return (
      <div role={'my-cmp'} className={innerClasses.concat([...this.props.classes])}>
        Hello world
      </div>
    );
  }
};

_Nicer and simpler way:_

class MyComponent {
  render () {
    var innerClasses = [
      'cmp--default',
      'cmp--mobile'
    ];  

    // Still renders <div role="my-cmp" class="cmp--default cmp--mobile contextual-styling-."> ... </div>
    // Includes inner classes and implicitly adds the inherited class' names from ParentComponent
    return (
      <div role={'my-cmp'} className={innerClasses}>
        Hello world
      </div>
    );
  }
};

I would love to hear your opinions on that. We needed such functionality in the project and ended up passing the inherited class' names by props, which feels a little inelegant.

@quantizor
Copy link
Contributor

What if you wanted to override it though? I think in this situation it's better for React to be less opinionated and leave it up to the implementor.

It would be pretty straightforward to implement this as a mixin, too.

@jimfb
Copy link
Contributor

jimfb commented Jul 10, 2015

Yeah, we strive to implement the most general and explicit solutions, so long as we don't introduce excessive burden on the developer. A single call to concat when you want to merge properties does not constitute excessive burden. Automatically merging styles is particularly bad if the MyComponent creates a wrapper div (for instance) and the styles are intended to apply to the nested element instead of the wrapper.

I'm going to close this out, since I'm fairly certain this is not something we want to do. Feel free to continue the discussion on this thread.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants