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

Remove css px warning, stop appending px to strings #6899

Merged
merged 1 commit into from
May 31, 2016

Conversation

jimfb
Copy link
Contributor

@jimfb jimfb commented May 27, 2016

cc @spicyj @zpao can one of you stamp?

expect(CSSPropertyOperations.createMarkupForStyles({
margin: '16 ',
left: '16 ',
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why change the property names?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Eeh, setting margin to a unitless 16 felt weird. I can change them back I suppose.

Copy link
Member

Choose a reason for hiding this comment

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

No weirder… you would probably never realistically set left to a unitless 16 either.

@sophiebits
Copy link
Collaborator

Did you fix up Facebook?

@zpao
Copy link
Member

zpao commented May 27, 2016

I just landed #6677 (so I wouldn't have to do something in just the branch) so you'll have to rebase too. Sorry.

@jimfb
Copy link
Contributor Author

jimfb commented May 27, 2016

Ok, fixed+rebased. Also I have a diff up for FB internally. After that merges, we will want to do one last pass through the scuba logs immediately prior to doing the sync.

expect(CSSPropertyOperations.createMarkupForStyles({
margin: '16 ',
opacity: 0.5,
padding: ' 4 ',
})).toBe('margin:16px;opacity:0.5;padding:4px;');
})).toBe('left:16;opacity:0.5;right:4;');
Copy link
Collaborator

Choose a reason for hiding this comment

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

pretty sure this won't pass

@jimfb jimfb force-pushed the remove-unitless-warning branch 4 times, most recently from 840afe9 to a1e5193 Compare May 28, 2016 12:05
@ghost
Copy link

ghost commented May 28, 2016

@jimfb updated the pull request.

@ghost
Copy link

ghost commented May 28, 2016

@jimfb updated the pull request.

@alexzherdev
Copy link
Contributor

I was working on this little fix #6928 when I stumbled on this PR. It looks like this one is scheduled for the next major release, but I'm not sure how versions are handled. Is my PR not necessary at all then?
Also, with these changes, wouldn't content: ' ' be trimmed to content: '' at https://github.com/facebook/react/pull/6899/files#diff-8e4557cf7e720e2a32380dc86168314aR49?

@ghost
Copy link

ghost commented May 31, 2016

@jimfb updated the pull request.

@jimfb jimfb merged commit 51f8b1b into facebook:master May 31, 2016
@bvaughn bvaughn mentioned this pull request Aug 1, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants