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

Styled Decorator #537

Closed
ozyman42 opened this issue Feb 28, 2017 · 15 comments
Closed

Styled Decorator #537

ozyman42 opened this issue Feb 28, 2017 · 15 comments

Comments

@ozyman42
Copy link

ozyman42 commented Feb 28, 2017

Could you add in the following code where appropriate:

styled.decorator = (strings, ...values) => (Component) => styled(Component)(strings, ...values);

So that we can begin using styled-components in the following way:

@styled.decorator`
    padding: 10px;
    background: blue;
    font-size: 12px;
`
export class SomeThing extends React.Component {
    render() {
        return <div className={this.props.className}>Hello World</div>;
    }
}

It would look much cleaner than writing:

class SomeThingComponent extends React.Component {
    render() {
        return <div className={this.props.className}>Hello World</div>;
    }
}

export const SomeThing = styled(SomeThingComponent)`
    padding: 10px;
    background: blue;
    font-size: 12px;
`
@k15a
Copy link
Member

k15a commented Feb 28, 2017

In my opinion there is no need to support a HOC or a decorator because that's a pattern we don't recommend to use with styled-components. What speaks against this version for you?

const Text = style.div`
    padding: 10px;
    background: blue;
    font-size: 12px;
`

export class SomeThingComponent extends React.Component {
    render() {
        return <Text>Hello World</Text>;
    }
}

This way you don't need to pass a classname down to a component which is a cleaner way to use styled-components.

@ozyman42
Copy link
Author

ozyman42 commented Feb 28, 2017

Sometimes one might not want to create an entirely separate component class if it will only be used in that one place. Also imagine if the styled component takes say 10 different arguments which would determine the style. In that case: which would be easier:
This:

const Text = styled.div`
    height: ${{height} => height}px;
    width: ${{width} => width}px;
    color: ${{color} => color};
    position:  absolute;
    top: ${{offset} => offset * 2}px;
    left: ${{offset} => offset * 4 + 3}px;
`

class SomeThingComponent extends React.Component {
    render() {
        return <Text height={this.props.height} width={this.props.width} color={this.props.color} offset={this.props.offset}>Hello World</Text>;
    }
}

Or this:

@styled.decorator`
    height: ${{height} => height}px;
    width: ${{width} => width}px;
    color: ${{color} => color};
    position:  absolute;
    top: ${{offset} => offset * 2}px;
    left: ${{offset} => offset * 4 + 3}px;
`
class SomeThingComponent extends React.Component {
    render() {
        return <div className={this.props.className}>Hello World</div>;
    }
}

@k15a
Copy link
Member

k15a commented Feb 28, 2017

You can also spread the props into the Text component:

class SomeThingComponent extends React.Component {
    render() {
        return <Text {...props}>Hello World</Text>;
    }
}

Which makes the first example of you not much longer or complicated then the example with the decorator.

@ozyman42
Copy link
Author

That may be so, but that doesn't solve the problem of needing to create an entirely separate component class. Someone might not want to do that.

@ozyman42
Copy link
Author

Your solution also creates another problem. What if within those props, there is a onClick function passed, and the developer wants that function bound on something other than the Text component.
With the decorator solution, they could do the following:

@styled.decorator`
    ...
`
class SomeThing extends React.Component {
    render() {
        return <div className={this.props.className}><button onClick={this.props.onClick}>Hello World</button></div>
    }
}

but with your solution:

class SomeThing extends React.Component {
    render() {
        return <Text {...props}><button onClick={this.props.onClick}>Hello World</button></Text>;
    }
}

this would have unintended consequences.

@k15a
Copy link
Member

k15a commented Feb 28, 2017

This would be the solution:

class SomeThing extends React.Component {
    render() {
        const { onClick, ...props } = this.props
        return <Text {...props}><button onClick={onClick}>Hello World</button></Text>;
    }
}

I don't think we will support decorators because there was already a discussions about HoC. @mxstbr said at #316 (comment) that we will not support them with the explanation

that the core idea behind styled-components is that you couple DOM nodes to their styles to enforce the presentational/container component split.

Because decorators are basically HoCs I think we will not support them as well.

On the other hand the HoC / decorator is a single line of code so if you really want to use it you should write your own little helper function.

@ozyman42
Copy link
Author

ozyman42 commented Mar 1, 2017

If the decorator would only be one line, why not support that one line? It shouldn't be too big of an increase to maintain. I can maintain it if nobody else wants to.

If the answer to that question is the philosophy for styled-components dictated by @mxstbr above, then why are you supporting this?

styled(SomeThing)`
    color: black;
    margin-left: ${props => props.leftMargin}px;
`

This is basically creating a higher-order component.
From Facebook's definition: a higher-order component is a function that takes a component and returns a new component.

@xcoderzach
Copy link
Contributor

That may be so, but that doesn't solve the problem of needing to create an entirely separate component class. Someone might not want to do that.

Why is creating a separate component a problem that needs to be solved? Performance issue? Code aesthetics? Just trying to understand the motivation 😄

@ozyman42
Copy link
Author

ozyman42 commented Mar 2, 2017

Code aesthetics and readability.

@ozyman42
Copy link
Author

ozyman42 commented Mar 2, 2017

I can create the PR.

@mxstbr
Copy link
Member

mxstbr commented Mar 3, 2017

I don't think we're going to support this officially I'm afraid, for the reasons stated above. I'm not convinced this adds any value to the library, in fact I think it takes away from the value of using it.

@mxstbr mxstbr closed this as completed Mar 3, 2017
@maxs15
Copy link

maxs15 commented Mar 24, 2017

I really like the idea as well 👍 it would make the code cleaner (less components to declare).
And it would be something optional.. available to decorators fans if they want it

@oleksii-udovychenko
Copy link

I also like the idea. It can be a sepparate package on NPM that adds more sugar to styled-components.

@pigulla
Copy link

pigulla commented Jun 1, 2017

FWIW, I just do this:

// with_styles.js
import styled from 'styled-components';

export default function (strings, ...items) {
    return function (WrappedComponent) {
        return styled(WrappedComponent)(strings, ...items);
    };
}
import with_styles from './with_styles';

@with_styles`
    border: 2px dotted lime;
`
class MyComponent extends React.PureComponent {
    // ...
}

@kitten
Copy link
Member

kitten commented Jun 1, 2017

It's simply not possible and not supported, because it would allow users to easily go against or enforced best practices and methodologies.

By default we do offer passing in any component into the styled factory function, and that can be used as an escalation e hatch or even to make this possible with a helper, but it's mainly for complex stacks of components and third party components.

It's not recommended to use this frequently, as building up your component structures from bottom to top leads to cleaner architectures that scales. At the bottom you'll find pure presentational components, above them structural ones, and at the top container/smart components. This can of course be nested and combined in different ways.

But by allowing this pattern by default, we'd be recommending to put the presentational components, like a stylesheet, at any position of this component stack, which is less desirable.

So if you really want to do this, we're not stopping you. But try to keep this in mind, and we believe you'll likely find more scalable styling code as a result.

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

8 participants