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

HTMLtoJSX issue with line-height style #53

Open
ghost opened this issue Feb 28, 2016 · 10 comments
Open

HTMLtoJSX issue with line-height style #53

ghost opened this issue Feb 28, 2016 · 10 comments
Labels

Comments

@ghost
Copy link

ghost commented Feb 28, 2016

React has shorthand for specifying pixel values in style. So when you put into HTMLtoJSX something like this
<div style="line-height: 50px;"></div>'
it produces
<div style={{lineHeight: 50}}>
The problem is that line-height acceptes plain values (like line-height: 50), so the above JSX is actually sets line-height style for 50, not 50px.

@Daniel15 Daniel15 added the bug label Apr 18, 2016
@Daniel15
Copy link
Member

Good catch, we probably need to special-case line-height. May need some special case here: https://github.com/reactjs/react-magic/blob/master/src/htmltojsx.js#L127-L129

@Daniel15
Copy link
Member

Daniel15 commented Apr 25, 2016

Should determine where React pulls this blacklist from: http://facebook.github.io/react/tips/style-props-value-px.html

@ssorallen
Copy link
Contributor

@Daniel15 Why modify those values at all? React 16 will stop appending 'px' for CSS values, so that modification will potentially break. Can HTMLtoJSX simply use the values verbatim?

@ssorallen
Copy link
Contributor

https://facebook.github.io/react/blog/2016/04/07/react-v15.html#new-helpful-warnings

React DOM: When specifying a unit-less CSS value as a string, a future version will not add px automatically.

@Daniel15
Copy link
Member

Daniel15 commented Apr 25, 2016

@ssorallen - That changelog entry only applies to strings (eg. {{width: '123'}}, not numbers (eg. {{width: 123}}). The behaviour for numbers remains unchanged.

Having said that, there's no harm in keeping the px suffix, so maybe HTMLtoJSX should just use it verbatim.

@sizer
Copy link

sizer commented Feb 12, 2018

any news about this issue?

@Daniel15
Copy link
Member

@mrdShinse I don't think anyone has submitted a PR to fix it yet

@drax10
Copy link
Contributor

drax10 commented Nov 19, 2018

@ssorallen - That changelog entry only applies to strings (eg. {{width: '123'}}, not numbers (eg. {{width: 123}}). The behaviour for numbers remains unchanged.

Having said that, there's no harm in keeping the px suffix, so maybe HTMLtoJSX should just use it verbatim.

If this is the case, this can be solved simply by removing these lines https://github.com/reactjs/react-magic/blob/master/src/htmltojsx.js#L727-L729 along with the isConvertiblePixelValue function. Please advise if I'm missing some reason some styles need the px suffix omitted.

drax10 added a commit to drax10/react-magic that referenced this issue Dec 2, 2018
Will address line-height style bug discussed in reactjs#53
Daniel15 pushed a commit that referenced this issue Dec 2, 2018
Will address line-height style bug discussed in #53
@SkyzohKey
Copy link

ESLint rule so this behavior can be prevented.

{
  ...
  "rules": {
    "no-restricted-syntax": [
      "error",
      {
        "selector": "Property[key.name=lineHeight] > Literal[value=/([0-9]+)[^px]$/]",
        "message": "Make sure to specify an unit when using lineHeight (see React issue: https://github.com/reactjs/react-magic/issues/53)"
      }
    ]
  }
}

@zomars
Copy link

zomars commented May 30, 2022

I'm willing to make a PR if someone can tell me where I can add an exception for line-height.

EDIT: I see that you have this method to strip px values but I can't where is being used:

function isConvertiblePixelValue(value) {

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

6 participants