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

Discuss alternate way to apply props to css attribute using bindProp #532

Closed
leonwilly opened this issue Feb 24, 2017 · 4 comments
Closed

Comments

@leonwilly
Copy link

import styled, {bindProp} from 'styled-components';

// rough implementation details 'utils/bindProp.js'
//
// class BoundProperty {
//     name: string;
//     styleOnly: boolean;
//     defaultValue: Object;
//
//     constructor (name: string, styleOnly: boolean = false) {
//         this.name = name;
//         this.styleOnly = styleOnly;
//     }
//
//
//     // Optional just an idea
//     default(value: Object): BoundProperty {
//         this.defaultValue = value;
//         return this;
//     }
//
// }
//
//
// export default function bindProp(name: string, styleOnly: boolean): BoundProperty {
//     return new BoundProperty(name, styleOnly);
// }
//
// Inside the template interpolation function do an instanceof BoundProperty check.
// If true lookup the property name in the props object and insert the value.
// If value is undefined use defaultValue
// If the styleOnly flag is true remove the property from the props object after all attributes have been updated.

const Progress = styled.div`
    width: ${(props) => props.width};
    /* or */
    width: ${bindProp('width', true).default('100%')};
`;

This solves the issue of unwanted style properties being passed to components. It could also slightly improve performance by reducing function calls during property updates. Just an idea I thought I would share.

@jacobp100
Copy link
Contributor

jacobp100 commented Mar 2, 2017

I did think about something like this—you could make a proxy that records property access. It came down to an issue a single issue, which I think yours will still suffer from.

Just for example, we'll say you don't pass down any styling props. Now say you want to allow computed properties (which seems to be a common use case). This might look like this,

const FineSoFar = styled.div`
  color: ${bindProp('active', active => active ? 'red' : 'blue')};
`

But what if your conditional now references another prop,

const Broken = styled.div`
  color: ${bindProp('active', active => active ? bindProp('primary') : bindProp('secondary')};
`

Until Broken has had all values, you do not know all styling props. This means that until you have exhausted all permutations of props, you will be passing down non-styling props.

I.e. if you do<Broken primary="red" secondary="blue" />, you'll pass down secondary, then when you update it to <Broken active … />, it'll correct it to not pass down any props.

@leonwilly
Copy link
Author

leonwilly commented Mar 3, 2017

@jacobp100 You make an excellent point. How about this instead

const Button = styled.div`
    ${bindProps("active", "primary", "secondary")}
    color: ${(props) => props.active ? props.primary : props.secondary};
`

One declaration at the top. It would simply populate a list upon interpolation and substitute its value as an empty string or be skipped. It requires no custom css addons.

You could even take this further and make declaring style props more cohesive.

const Button = styled.div`
    ${stylePropTypes({
        active: React.PropTypes.bool,
        primary: React.PropTypes.string,
        secondary: React.PropTypes.string})}
    ${defaultStyleProps({
        active: false,
        primary: "blue",
        secondary: "gray"})}
    color: ${(props) => props.active ? props.primary : props.secondary};
`

Declaring a class as a string template essentially. You could drop the "stylePropTypes" call and simply detect if the object is describing the types of props or setting the props by doing instance checks on the values inside.

const Button = styled.div`
    ${{
        active: React.PropTypes.bool,
        primary: React.PropTypes.string,
        secondary: React.PropTypes.string}}
    ${{
        active: false,
        primary: "blue",
        secondary: "gray"}}
    color: ${(props) => props.active ? props.primary : props.secondary};
`

Merge the two objects keys together and you'll have a list of properties strictly for style.

@jacobp100
Copy link
Contributor

I'll copy this over into the other thread!

@leonwilly
Copy link
Author

Cool thank you.

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