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 support for proper styles as objects #280

Merged
merged 6 commits into from
Dec 17, 2016
Merged

Add support for proper styles as objects #280

merged 6 commits into from
Dec 17, 2016

Conversation

mxstbr
Copy link
Member

@mxstbr mxstbr commented Dec 1, 2016

For making polished a cross-library thing we need to have support for media queries, pseudo selectors etc in interpolated style objects. (this is literally a 9 line change 👍)

const Box = styled.div`${{
  backgroundColor: 'red',
  '@media screen and (min-width: 800px)': {
    backgroundColor: 'blue',
  },
}}`

Also adds tests to verify the new feature.

/cc @bhough @steida @gaearon

Please review @geelen, I might've missed some edge case

For making `polished` a cross-library thing we need to have support for
media queries, pseudo selectors etc in inline style objects.

This PR adds support for that.
@mxstbr
Copy link
Member Author

mxstbr commented Dec 1, 2016

This doesn't support anything like arrays or functions in objects though, this doesn't work:

const Box = styled.div`${{
  backgroundColor: (props) => props.bg, // ⚠ Doesn't work
}}`

I don't know if we need to add support for that?

@bhough
Copy link
Contributor

bhough commented Dec 1, 2016

I don't know if we need to add support for that?

I don't think we need to. Feels like that is a step too far as far as not encouraging people to primarily use objects with styled components.

@k15a
Copy link
Member

k15a commented Dec 2, 2016

Hey, I am just wondering why you choose this syntax. I would favour this syntax:

styled.div((props) => ({
  backgroundColor: props.bg, // Does work
}))

or this:

styled.div({
  backgroundColor: props.bg, // Does work
})

over the current one. You could even support both.

The downside would be that you can't mix normal styled-components with the style objects like:

const Box = styled.div`
color: blue;
${{
  backgroundColor: 'red'
}}`

Could you please explain why you choose to go with the current syntax?

@mxstbr
Copy link
Member Author

mxstbr commented Dec 2, 2016

Could you please explain why you choose to go with the current syntax?

It's necessary to support this syntax for ReactNative. You wouldn't use this syntax manually, you'd just interpolate existing ReactNative styles in a legacy project:

const oldStyles = {
  color: 'red',
  background: 'blue',
}

const NewStyledComponentsComponent = styled.View`
  ${oldStyles}
  flex: 1;
`

That's why we need to support this. This might not be the main interface for styles as objects, and we might add one similar to your ideas! Haven't thought too much about that yet, as this PR is solely to add support for pseudos and media queries for pre-existing projects.

@mxstbr mxstbr modified the milestone: v1.2 Dec 17, 2016
@mxstbr
Copy link
Member Author

mxstbr commented Dec 17, 2016

Going to ship this in an effort to release a new version!

@mxstbr mxstbr merged commit 1085218 into master Dec 17, 2016
@mxstbr mxstbr deleted the object-notation branch December 17, 2016 10:21
@geelen
Copy link
Member

geelen commented Dec 18, 2016

Sorry I missed this. This is 👍

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.

4 participants