styled-components / styled-components

Visual primitives for the component age. Use the best bits of ES6 and CSS to style your apps without stress 💅
https://styled-components.com
MIT License
40.46k stars 2.5k forks source link

styled-component wrap Component flow-type check isn't active #1094

Closed zenjava closed 7 years ago

zenjava commented 7 years ago

Version

2.1.0

Steps to reproduce

run the code.

Expected Behavior

//@flow
interface LogoPropsDefault {
  /* This prop is optional, since TypeScript won't know that it's passed by the wrapper */
}

interface LogoProps {
  /* This prop is optional, since TypeScript won't know that it's passed by the wrapper */
  className?: string;
  title: string;
}

class Logo extends React.Component<LogoPropsDefault, LogoProps, any> {
  static defaultProps: LogoPropsDefault;
  props: LogoProps;

  render () {
    return (
      <div className={this.props.title}>
         {this.props.title}
      </div>
    );
  }
}

const LogoStyled = styled(Logo)`
  ${props => props.className * 5}
  font-family: 'Helvetica';
  font-weight: bold;
  font-size: 1.8rem;
`;
const logoNormal = <Logo title={1}/>; // check className type is error
const logoStyled = <LogoStyled title={1}/>; // is right

Actual Behavior

//@flow
interface LogoPropsDefault {
  /* This prop is optional, since TypeScript won't know that it's passed by the wrapper */
}

interface LogoProps {
  /* This prop is optional, since TypeScript won't know that it's passed by the wrapper */
  className?: string;
  title?: string;
}

class Logo extends React.Component<LogoPropsDefault, LogoProps, any> {
  static defaultProps: LogoPropsDefault;
  props: LogoProps;

  render () {
    return (
      <div className={this.props.className}>
        {this.props.title}
      </div>
    );
  }
}

const LogoStyled = styled(Logo)`
  ${props => props.className * 5}
  font-family: 'Helvetica';
  font-weight: bold;
  font-size: 1.8rem;
`;
const logoNormal = <Logo title={1}/>; // check className type is error
const logoStyled = <LogoStyled title={1}/>; // check className type is error
kitten commented 7 years ago

The className prop in React in general can't be a number. Also it will be passed on to the child component, since you're wrapping a custom custom component here.

Apart from the instructions still being in your PR description here, we need more information on:

Also, can you update the title to be more descriptive please? It doesn't seem to match your issue :smile:

From what I can tell you were trying to use the className prop for a number. Use a different prop name instead :wink: Also note that wrapping your own components with StyledComponents is not considered best practice. Try to use presentational components (in our case StyledComponents) as leaf components that don't wrap anything, except for third-party components.

zenjava commented 7 years ago

Thanks, I am confused LogoStyled is wraping with StyledComponents, props type check, is not active on Flow-type checker.If I need wrapping my own components with StyledComponents, how do write it better and flow-typed is find the 'title' type is error for LogoStyled ?

kitten commented 7 years ago

You're talking about Flow? In your example you mention TypeScript:

/* This prop is optional, since TypeScript won't know that it's passed by the wrapper */

We assume that a specific structure, that adheres to our best practices, is used with the typings. So the best practice for your Logo component is more like this:

const LogoTitle = styled.div`
  /* this is an invalid rule that just adds a number to the CSS => invalid CSS will be generated */
  /* ${props => props.className * 5} */
  font-family: 'Helvetica';
  font-weight: bold;
  font-size: 1.8rem;
`;

// class is not needed but can be used
const Logo = ({ title }: LogoProps) => (
  <LogoTitle>
    {title}
  </LogoTitle>
);

As you can see I'm reversing the relationship here. LogoStyled (renamed to LogoTitle) is a presentational component. That means that all it does is create an element with some styling. The Logo component however is a structural component. This means that all it does is arrange/use some presentational components and pass through some data. Above them will be your container/smart components.

This structure is recommended and styled(X) is discouraged to be used with anything but third-party components. See docs: https://www.styled-components.com/docs/basics#styling-any-components

The styled method works perfectly on all of your own or any third-party components as well. [...] If you're using any external library, you can consider using this pattern to turn them into styled components.

I hope this also resolves your typings issue :smile: Btw if the issue persists we will need your flow/ts config.

cc @styled-components/typers

zenjava commented 7 years ago

Thank you, your answer solved my question. Once again thank you for your attention。 😄👍

kitten commented 7 years ago

Cool :)