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

Correctly trim strings for css properties #1642

Merged
merged 1 commit into from
Jun 26, 2014

Conversation

ryanseddon
Copy link
Contributor

This PR correctly trims property strings if passed in with a space. It also correctly matches unit less properties if the name is passed in as a 'string' and not camelCase by converting the string to camelCase.

  • If a unit less value is passed with a space it'll return 1 px
  • If passed 'z-index': 1 it'll return 'z-index': '1px' convert str camelCase when doin unitless lookup

See test case

Not applicable anymore see #1662

var Hello = React.createClass({
    render: function() {
        var styles = {
            'z-index': 1,
            margin: '10 '
        };
        return <div style={styles}>Hello {this.props.name}</div>;
    }
});

The following component yields inline styles: z-index:1px;margin:10 px;

@syranide
Copy link
Contributor

syranide commented Jun 4, 2014

'z-index' should be zIndex (and not just according to React, but according to the JavaScript DOM style object too).

@syranide
Copy link
Contributor

syranide commented Jun 4, 2014

Kind of related: #1357

@ryanseddon
Copy link
Contributor Author

Yeah I guess, but it's a pretty small amount of code to cater for both string and camelcase. Either way I can back it out if not.

@syranide
Copy link
Contributor

syranide commented Jun 4, 2014

@ryanseddon Ah my bad, I saw z-index:1px which obviously isn't correct and missed your point about actually adding camelCasing of hyphenated names. Anyway, I believe the discussion whether to support hyphenated names or not has been brought up before, but never really got decided I think (which is why both kind of work today).

Personally, I think it makes sense not to support hyphenated names as it would introduce ambiguity (if both are present) and unnecessarily complicates/breaks potential custom logic for modifying styles (like components could).

@ryanseddon
Copy link
Contributor Author

This PR covers the other issue of trimming strings so unitless values don't end up margin: 10 px; when there is a space in the value.

I honestly think this should cover handling camelCasing of string keys.

Do you want me to revert the camelCase changes and is the trimming code ok?

@syranide
Copy link
Contributor

syranide commented Jun 5, 2014

cc @zpao

@sophiebits
Copy link
Collaborator

(I'd personally prefer that we go the other way and not add px to strings that look like numbers, as I mentioned in #1357.)

@zpao
Copy link
Member

zpao commented Jun 22, 2014

re: hyphenated styles
I agree that we should only support 1 way. And currently that's camelCase. We should stick with that and warn (#1662).

re: trimming
If we stick with our current behavior of appending px, then what we have right now is a bug and we should trim. Short term we can take this part. Long term I think we should reconsider our + 'px' behavior entirely.

var isNonNumeric = isNaN(value);
if (isNonNumeric || value === 0 ||
isUnitlessNumber.hasOwnProperty(name) && isUnitlessNumber[name]) {
return '' + value; // cast to string
}

if(typeof value === 'string'){
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please match existing style. if (...) {

* If a unitless value is passed with a space it'll return `1 px`
@ryanseddon
Copy link
Contributor Author

I've backed out the camelCase code and fixed the if statement to match code style.

@ryanseddon ryanseddon changed the title Correctly trim strings for css properties, camcelCase props Correctly trim strings for css properties Jun 22, 2014
@syranide
Copy link
Contributor

@zpao Regarding the px-behavior, (just) an idea, what if we extended JSX with suffixes? Perhaps something like 30px, 40vh, (height)px (mathematical operations on them are not necessary as there exists no valid conversion between units, so it would just translate to '30px'). There shouldn't be any syntax ambiguity I believe (but perhaps in the future?). I guess more importantly, hypothetically, would we even want JSX to move in this direction? (i.e, be more than just JS + "React XML").

PS. I believe the newer C++ TRs uses this syntax too, including parenthesis (although with possibility for mathematical operations), so that's something at least.

@sophiebits
Copy link
Collaborator

@syranide I'm not sure what value that would give over just writing the strings verbatim?

@syranide
Copy link
Contributor

@spicyj Shorter and significantly less ugly, but yeah, it would just be sugar.

@zpao
Copy link
Member

zpao commented Jun 25, 2014

sugar for things that are already simple is usually overkill IMO. And at that point you're in what we think is JS already, so it's a bit awkward.

@zpao zpao self-assigned this Jun 25, 2014
zpao added a commit that referenced this pull request Jun 26, 2014
Correctly trim strings for css properties
@zpao zpao merged commit 3581a92 into facebook:master Jun 26, 2014
@zpao
Copy link
Member

zpao commented Jun 26, 2014

Thanks!

@ryanseddon ryanseddon deleted the trimCSSValues branch June 26, 2014 05:04
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

Successfully merging this pull request may close these issues.

4 participants