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

CSS property values that are strings shouldn't have 'px' appended #1357

Closed
sophiebits opened this issue Apr 5, 2014 · 9 comments
Closed
Assignees

Comments

@sophiebits
Copy link
Collaborator

It's weird that style={{margin: '42'}} gets turned into margin: 42px;. I think we should only add px if the value is an actual number.

@syranide
Copy link
Contributor

syranide commented Apr 6, 2014

I'll just chime in with what I mentioned in the chat last time, I think it's a great solution. Just as margin: '42px 42px 42px 42px' means you're providing a raw CSS-string, '42px' would mean you're providing a raw CSS-string. So it makes sense logically.

However, <Comp size={42}> and <Comp size="42"> suddenly has two very different outcomes if the component passes the value as-is to the style-object. I see no intuitive way out of this, sure enforcing numbers as {42} is technically the right solution, but it's not obvious to new users (document it thoroughly) and it's potentially error prone for a user base accustomed to HTML (especially designers I would assume).

I don't mind making {42} the only right way, but to me it seems like there's no way to enforce it at run-time without propTypes which is a developer burden, and as I've understood it, an entirely optional feature in your eyes. So it seems kind of fragile.

@zpao
Copy link
Member

zpao commented Oct 14, 2015

Note: this isn't actually fixed, just have a warning in place.

@zpao zpao reopened this Oct 14, 2015
@jimfb
Copy link
Contributor

jimfb commented Oct 14, 2015

Yep, was autoclosed by the PR.

Mayank1791989 pushed a commit to Mayank1791989/eslint-plugin-playlyfe that referenced this issue Aug 23, 2016
@viniciusdacal
Copy link

should we add a version in the warning message to this behavior be removed?

@SEAPUNK
Copy link

SEAPUNK commented Apr 14, 2017

So just to clarify, is passing a number value as string type the only thing that will no longer have px appended to it? Will passing a plain number a la <div style={{width: 40}} /> still be supported, or is there also an intent to remove that as well?

@viniciusdacal
Copy link

@SEAPUNK As I understood, only string type. The example you gave, will still be supported.

@aweary
Copy link
Contributor

aweary commented Sep 19, 2017

@gaearon we still have this warning in place, should we consider acting on for 16 or wait until 17?

@sophiebits
Copy link
Collaborator Author

@aweary Too late for breaking changes for 16, let's look at doing it in 17. Can probably land soon after the release behind a flag.

@aweary aweary self-assigned this Sep 19, 2017
@gaearon
Copy link
Collaborator

gaearon commented Oct 4, 2017

Hmm. I neither see a warning, nor do I see it apply the style.
I think this was fixed in 16.

https://jsfiddle.net/r44uryb1/

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

8 participants