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

Make themed values less ambiguous #6

Closed
sonhanguyen opened this issue May 28, 2019 · 10 comments

Comments

@sonhanguyen
Copy link

commented May 28, 2019

Hi, great library πŸ‘ , I have a

πŸš€ Feature Proposal

Instead of this:

  background-color: primary; /* ⟢ theme.colors.primary */
  margin: 2; /* ⟢ theme.space.2 */

maybe do this

  background-color: $primary; /* ⟢ theme.colors.primary */
  margin: $2; /* ⟢ theme.space.2 */

Pitch

Pros:

  • Easier to read, it's immediately obvious what comes from theme and what's not
  • Easier to refactor, grep, codemode, etc, less chance to do something by mistake
  • Familiar to users of sass, less etc, they probably could just copy-paste code over in many cases.

Cons:

  • Have to type one more character
@sonhanguyen

This comment has been minimized.

Copy link
Author

commented May 28, 2019

Actually, this should be a breaking change if accepted but I think it's worth it.

@neoziro

This comment has been minimized.

Copy link
Member

commented May 28, 2019

Hello @sonhanguyen, I understand your point and your pros. But there is one thing very different from Sass, it is not a variable:

width: 0.123;

This value will be transformed into 12.3%, it is confusing to have a $ in front of a value isn't it?

Maybe we could have the best of two world, being able to specify "$" or to not specify it. What do you think?

@sonhanguyen

This comment has been minimized.

Copy link
Author

commented May 28, 2019

I think one of the motivations is to reduce the chance of mistake. If we do introduce $ but also implicitly lookup when there is no $, it's gonna even increase the confusion and doesn't really prevent mistakes.

Per your point about the value conversion, I'm not aware it's a feature and I like to think about it as a separate thing from themed value lookup. So yes a plain number should just be width: 0.123 but a variable lookup should be width: $large.

I'm not a fan of functional css approaches like styled-system so I rely a lot on themed values, that's where your library could really help, since it makes heavy use of themed values less noisy. However, the ambiguity is really holding me back, to be honest.

@sonhanguyen

This comment has been minimized.

Copy link
Author

commented May 28, 2019

It can be a global option that once turned on you have to lookup with $ everywhere. So either use $ or not use $, no mixing. That could work πŸ˜„

@neoziro

This comment has been minimized.

Copy link
Member

commented May 28, 2019

I will think about it, let's wait for community opinion.

@Errindel

This comment has been minimized.

Copy link

commented Jun 11, 2019

Hi,
we started to use it in our teams, and from my point of view the syntax is very clean. When you write styled-components you have to know that there is a theme that defines some keys or keywords. For me there is no possibility to misunderstand e.g. 'primary'.

I personally vote for leaving the syntax as it is.

I'll post an update when more devs will have their opinion on this topic.

@neoziro

This comment has been minimized.

Copy link
Member

commented Jun 11, 2019

Same, at Smooth Code, we use it and it is not confusing. It depends of your theme values. Also you are able to specify $primary in the theme. So it is up to you!

@sonhanguyen

This comment has been minimized.

Copy link
Author

commented Jun 11, 2019

Same, at Smooth Code, we use it and it is not confusing. It depends of your theme values. Also you are able to specify $primary in the theme. So it is up to you!

good point

@jamesg1

This comment has been minimized.

Copy link

commented Jun 23, 2019

Great library, I would say no to making it an extra char like $ for variables. Xstyled is just shorthand extending SC/Emotion. When your writing theme/css your always referencing back to your theme so adding a $ doesn't add much value. What if you wanted to reference things down the theme tree - $primary['$var2'] doesn't really make sense when you could do primary.var2.

@neoziro

This comment has been minimized.

Copy link
Member

commented Jun 24, 2019

Yes it will not happen, if you want "$", then feel free to prefix all variables in your theme (you can also write an helper that prefixes all variables). It will not be part of xstyled core, but feel free to create your own convention.

@neoziro neoziro closed this Jun 24, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.