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

Add defaultProps using the options argument in styled #617

Closed
wants to merge 3 commits into from

Conversation

tkh44
Copy link
Member

@tkh44 tkh44 commented Apr 5, 2018

What:
You can specify default props in the options argument of styled

const MyComponent = styled('input', {
  props: {
    'type': 'checkbox'
  }
})({
  height: 40,
  width: 40,
  color: 'green'
})

Why:
I got annoyed.

How:
I did a simple merge in render. I may be missing something obvious but this should work fine.

Checklist:

  • Documentation
  • Tests
  • Code complete

@tkh44 tkh44 requested a review from emmatown April 5, 2018 22:25
@ChristopherBiscardi
Copy link
Member

Cool, Seems fine to me (and useful). Test failures seem to be trivial type issues.

docs/styled.md Outdated

```jsx live
const MyComponent = styled('input', {
props: {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems like defaultProps instead of just props would nicely relate to true React APIs

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with props not being optimal. I like @mitchellhamilton's idea of using withProps to match recompose.

@@ -85,7 +88,9 @@ function createEmotionStyled(emotion: Emotion, view: ReactType) {
}
}
render() {
const { props, state } = this
const state = this.state
const props = Object.assign({}, defaultProps, this.props)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just a thought - it generally should be the same, but maybe worth to offload that work to builtin React's mechanism of defaultProps? u could just assign static defaultProps = options.props

Copy link
Member Author

@tkh44 tkh44 Apr 6, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test would fail I believe if we just used defaultProps on the component.
https://github.com/emotion-js/emotion/pull/617/files#diff-77b4dbda613d78e500ded003e8362f64R1125

Copy link
Member Author

@tkh44 tkh44 Apr 6, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe you are onto something though. Maybe there is a need for defaultProps and withProps. defaultProps seems to be covered by the React API like you said.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tkh44 I don't think it would fail, the example below would fail with defaultProps though and probably does with this current implementation as well

const SomeComponent = styled('div', { props: { className:'thing' }})(...styles)

render(<SomeComponent className="other-thing" />)

@diegohaz
Copy link
Contributor

diegohaz commented Apr 5, 2018

Why not this?

const MyComponent = styled('input')({
  height: 40,
  width: 40,
  color: 'green'
})

MyComponent.defaultProps = {
  'type': 'checkbox'
}

@Andarist
Copy link
Member

Andarist commented Apr 5, 2018

@diegohaz I guess just convenience - personally I use what u've shown and im just fine with it. It's easily doable in userland and doesnt cost anything in the library (because there is nothing special in it for that case) - actual implementation (as we can see in this PR) is rather minimal though, so from my point of view 🤷‍♂️

@ChristopherBiscardi
Copy link
Member

I saw some conversation on Twitter about possibly making this a function, so here's a (possible?) use case where I do this currently to make styled work with Formik:

const TextInput = styled.input`
  outline: none;
  padding: 11px 10px;
  border: 1px solid #d4d4d2;
  border-radius: 3px;
  box-shadow: none;
  transition: border 0.3s, color 0.3s;
  margin-bottom: ${({ hasError }) => (hasError ? "8px" : bottomMargin)};
`;

const FormikTextInput = ({ field, ...props }) => <TextInput type="text" {...field} {...props} />;

but it might be nice to

const FormikTextInput = styled('input', ({ field, ...props }) => {
    type:'text',
    ...field,
    ...props
  })`
  outline: none;
  padding: 11px 10px;
  border: 1px solid #d4d4d2;
  border-radius: 3px;
  box-shadow: none;
  transition: border 0.3s, color 0.3s;
  margin-bottom: ${({ hasError }) => (hasError ? "8px" : bottomMargin)};
`;

both used as

render={({ errors, touched, isSubmitting }) => (
...
    <Field
      component={FormikTextInput}
      name="name"
      placeholder="Give the query a descriptive name"
      hasError={touched.name && errors && errors.name}
      disabled={isSubmitting}
    />

(disclaimer, I haven't thought this through that much. Just pulled some code out of a codebase I had).

@diegohaz
Copy link
Contributor

diegohaz commented Apr 5, 2018

We had a discussion about it on styled-components. I always vote for keeping public API as simple as possible, specially when it can already be achieved in another simple way.

Less bugs, less things to maintain, less things to upgrade in future versions etc.

I see some value on it If you guys are planning to accept functions that return default props based on actual props. But I'd wait for more people to come up with use cases. 😝

@emmatown
Copy link
Member

emmatown commented Apr 5, 2018

I'm good with adding this in general. I think we should probably call it withProps and have a method like withComponent but withProps to align with glamorous and IMO the method is more convenient but let's have both and implement the method with the option.
Let's definitely allow functions.
Let's just make it the same as glamorous.
https://github.com/kentcdodds/glamorous-website/blob/master/pages/api/content/glamorous.md#withprops

also, this doesn't handle this case

const _Button = styled("button", {
  props: {
    style: { color: "hotpink" }
  }
})`
  color: turquoise;
`;

const Button = styled(_Button)();

@tkh44
Copy link
Member Author

tkh44 commented Apr 6, 2018

I'm good with adding this in general. I think we should probably call it withProps and have a method like withComponent but withProps to align with glamorous and IMO the method is more convenient but let's have both and implement the method with the option.

Let's definitely allow functions.

How many recompose apis do you think we could provide legit use without ballooning the size? (https://github.com/acdlite/recompose/blob/master/docs/API.md#withprops)

Let's just make it the same as glamorous.
kentcdodds/glamorous-website:pages/api/content/glamorous.md@master#withprops

I agree.

@tkh44
Copy link
Member Author

tkh44 commented Apr 6, 2018

@ChristopherBiscardi thats pretty much what I had in mind. (https://github.com/acdlite/recompose/blob/master/docs/API.md#withprops)

@diegohaz I see what you mean in this case

const MyComponent = styled('input')({
  height: 40,
  width: 40,
  color: 'green'
})

MyComponent.defaultProps = {
  'type': 'checkbox'
}

In the case of className though we need to merge them in a special way. The default prop would never appear because we apply a class name ourselves so the value in defaultProps will never apply

const MyComponent = styled('input')({
  height: 40,
  width: 40,
  color: 'green'
})

MyComponent.defaultProps = {
  'className': 'marketing-needs-this-😖-class-name'
}

@@ -76,12 +97,6 @@ const Section = styled('section')`
`
// Create an aside element with the same styles as Section
const Aside = Section.withComponent('aside')
render(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The render function is needed to render the example.

@browniefed
Copy link

browniefed commented Apr 6, 2018

If the props is a function can theme be injected? I think that would make this more useful than a generic defaultProps assignment and or functional component

My use case deals with styled-system.
I want to grab a prop named color, grab it off of themes.color then pass in as an SVG fill prop.

@Andarist
Copy link
Member

Andarist commented Apr 6, 2018

In the case of className though we need to merge them in a special way. The default prop would never appear because we apply a class name ourselves so the value in defaultProps will never apply

Isnt className just handled in special way in render?

I don't see how it would never apply with defaultProps approach.

@emmatown
Copy link
Member

I've been thinking about this quite a bit and I think we shouldn't add it and instead encourage people to create normal components and use the css prop. For example, the example given could instead be

const MyComponent = props => (
  <input
    type="checkbox"
    css={{
      height: 40,
      width: 40,
      color: 'green'
    }}
    {...props}
  />
)

I think encouraging this is better because it only requires learning one Emotion specific API and the rest of it is plain React. Also, we don't have to think about supporting a bunch of different edge cases like functions.

To anyone who tries the example with emotion 9 and class names don't get passed through, that's known and will be fixed in emotion 10

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

Successfully merging this pull request may close these issues.

6 participants