-
Notifications
You must be signed in to change notification settings - Fork 46.9k
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
No unit-less CSS value warning for value: "0"
#6458
Conversation
Correct. We should not fire the warning for zeros, because zeros are zero no matter what the units. |
@zpao This one should also be safe for v15.0.1, though not super high priority if you want to punt it to a larger release that has been tested more heavily internally. |
Hmm, I'm not quite sure this is technically safe in 15 at all as it changes the value we set for the style. I'm not really sure we can say this is a bug fix since it doesn't fix a bug. If we simply wanted to silence the warning we should be exempting |
@zpao Na, that's just a bug fix. Rendering 0px works in many browsers, but not all. Rendering "0" is always the correct behavior, but rendering "0px" is sometimes incorrect/broken and will result in complaining browsers (Opera?). I don't think it's possible for someone to depend on it rendering "0px" as the intended behavior of their application. |
@zpao This commit does change the value in style, but in a good way. Before the commit:
After:
If you simply exempting |
Try it in Opera. If Opera doesn't fail/complain, try an older version of Opera. I'm nearly certain that I've seen |
@jimfb can you point me to a specific case where "0" is ok but "0px" is not? (not counting the exemptions we already have for some properties). @mondaychen I'm agree that the change is better, my question is about whether it's the right change to take in 15. We will change the behavior on string numbers, that's the point of the warning. But we haven't done it yet for any number so should we be doing it for "0" now? Or should we exclude at the spot we warn? Keep in mind the way you phrased the pull request and description - your goal as I interpreted it was to exclude that case from the warning, not to change the generated value. |
@jimfb what version of Opera is that for and can you reproduce? I don't recall any issue reported to us about this in the history of React. So as reported and per the intent here, this is not a bug fix but a behavior change. |
Honestly, I don't remember. I spent a few minutes searching the internet, but came up empty. Maybe it's not a problem. Probably not a good use of time to keep searching / trying to repro. I just distinctly remember running into this CSS issue when I found the solution, being like "oh, yeah, zero is always zero regardless of units, I suppose that makes sense, but seems like a really stupid css/browser requirement". I distinctly remember css units being required for non-zero but breaking for zero, but this was a couple years ago and I unfortunately don't remember the details. |
@zpao I still believe this is pretty safe despite changing the return value. But I'm OK if exempting |
What is the status of this? I'm still getting the warning with React 15.1.0. Has this been bumped to React 16.x milestone? If so, why, and how long are we going to have to wait for the 16.x release? |
I believe you can find the discussion “why” in the comments right above: #6458 (comment) and following. A pull request that implements the same in a different way (without behavioral changes) is #6677, and I think it will be out in the next patch release. |
After upgrading to React v15.0 I got a lot of warnings like:
I can avoid the warning by changing
"0"
to number0
or"0px"
. I like neither ways because:I believe this warning is overall a very good thing but I want to keep my current way of writing string zeros in CSS values.