Skip to content

Individual border properties should reference theme values #276

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

Closed
dburles opened this issue Aug 7, 2019 · 10 comments
Closed

Individual border properties should reference theme values #276

dburles opened this issue Aug 7, 2019 · 10 comments

Comments

@dburles
Copy link
Contributor

dburles commented Aug 7, 2019

I'd be happy to create a PR for this, if it makes sense. For example, this works just fine:

borderWidth: 'thin',
borderStyle: 'solid',
borderColor: 'primary',

however, this will result in the literal values winding up in the CSS:

borderBottomWidth: 'thin',
borderBottomStyle: 'solid',
borderBottomColor: 'primary',

I think it would make sense for the API to support all possible values:

X = Top, Bottom, Left, Right

borderXWidth
borderXColor
borderXStyle
X = Top, Bottom

borderXLeftRadius
borderXRightRadius
@dburles
Copy link
Contributor Author

dburles commented Aug 7, 2019

PR styled-system/styled-system#671 :)

@kutyel
Copy link

kutyel commented Aug 7, 2019

In my case, when I do the following: border: '2px solid muted', where muted is a theme variable, it does not resolve it correctly, also, if I try borderBottomColor: 'muted' it does not work either, is this the same issue? :)

@dburles
Copy link
Contributor Author

dburles commented Aug 7, 2019

Yeah, that's right it won't resolve anything inside the shorthand string. You can either do something like:

border: theme => `${theme.borderWidths.thin}px ${theme.borderStyles.thick} ${theme.colors.muted}`

(which kinda sucks) or, a mix (which I think still has a bug if you order them incorrectly) such as:

border: theme => `${theme.borderWidths.thin}px ${theme.borderStyles.thick}`,
borderColor: 'muted'

or you can apply them individually:

borderWidth: 'thin',
borderStyle: 'thick',
borderColor: 'muted',

but it won't support borderBottomColor, which the PR addresses

@kutyel
Copy link

kutyel commented Aug 7, 2019

Awesome, that did the trick in the meantime, thanks! 🙌🏼

@jxnblk
Copy link
Member

jxnblk commented Aug 7, 2019

which I think still has a bug if you order them incorrectly

This isn’t a bug, this is due to the cascade and how CSS is designed to work FWIW

@dburles
Copy link
Contributor Author

dburles commented Aug 8, 2019

@jxnblk Cool thanks for clarifying, couldn't remember the exact details around it!

@dburles
Copy link
Contributor Author

dburles commented Aug 9, 2019

This is now supported in the latest version of @styled-system/css 🎉 run npm i theme-ui to update the dependency!

@dburles dburles closed this as completed Aug 9, 2019
@kutyel
Copy link

kutyel commented Aug 9, 2019

Now, this works:

  borderBottomWidth: '2px',
  borderBottomStyle: 'solid',
  borderBottomColor: 'muted',

but this still does not 😟 borderBottom: '2px solid muted'

@dburles
Copy link
Contributor Author

dburles commented Aug 9, 2019

@kutyel think about how the values are resolved, borderBottom references borders. borderBottomColor references color, borderBottomWidth references borderWidths. Passing in a string containing values that need to be resolved across different parts of the global theme object won't work. That's why for shorthand you need to provide a function to reference the theme:

borderBottom: theme => `${theme.borderWidths.thin}px ${theme.borderStyles.thick} ${theme.colors.muted}`

@jxnblk
Copy link
Member

jxnblk commented Aug 9, 2019

@kutyel see also #81

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