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.

Sign up for GitHub

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
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions src/renderers/dom/shared/__tests__/CSSPropertyOperations-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,12 +60,12 @@ describe('CSSPropertyOperations', function() {
})).toBe('left:0;margin:16px;opacity:0.5;padding:4px;');
});

it('should trim values so `px` will be appended correctly', function() {
it('should trim values', function() {
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.

opacity: 0.5,
padding: ' 4 ',
})).toBe('margin:16px;opacity:0.5;padding:4px;');
right: ' 4 ',
})).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

});

it('should not append `px` to styles that might need a number', function() {
Expand Down
43 changes: 0 additions & 43 deletions src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -173,49 +173,6 @@ describe('ReactDOMComponent', function() {
);
});


it('should warn about styles with numeric string values for non-unitless properties', function() {
spyOn(console, 'error');

var div = document.createElement('div');
var One = React.createClass({
render: function() {
return this.props.inline ?
<span style={{fontSize: '1'}} /> :
<div style={{fontSize: '1'}} />;
},
});
var Two = React.createClass({
render: function() {
return <div style={{fontSize: '1'}} />;
},
});
ReactDOM.render(<One inline={false} />, div);
expect(console.error.calls.count()).toBe(1);
expect(console.error.calls.argsFor(0)[0]).toBe(
'Warning: a `div` tag (owner: `One`) was passed a numeric string value ' +
'for CSS property `fontSize` (value: `1`) which will be treated ' +
'as a unitless number in a future version of React.'
);

// Don't warn again for the same component
ReactDOM.render(<One inline={true} />, div);
expect(console.error.calls.count()).toBe(1);

// Do warn for different components
ReactDOM.render(<Two />, div);
expect(console.error.calls.count()).toBe(2);
expect(console.error.calls.argsFor(1)[0]).toBe(
'Warning: a `div` tag (owner: `Two`) was passed a numeric string value ' +
'for CSS property `fontSize` (value: `1`) which will be treated ' +
'as a unitless number in a future version of React.'
);

// Really don't warn again for the same component
ReactDOM.render(<One inline={true} />, div);
expect(console.error.calls.count()).toBe(2);
});

it('should not warn for "0" as a unitless style value', function() {
spyOn(console, 'error');
var Component = React.createClass({
Expand Down
45 changes: 4 additions & 41 deletions src/renderers/dom/shared/dangerousStyleValue.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,10 +12,8 @@
'use strict';

var CSSProperty = require('CSSProperty');
var warning = require('warning');

var isUnitlessNumber = CSSProperty.isUnitlessNumber;
var styleWarnings = {};

/**
* Convert a value into the proper css writable value. The style name `name`
Expand Down Expand Up @@ -43,47 +41,12 @@ function dangerousStyleValue(name, value, component) {
return '';
}

var isNonNumeric = isNaN(value);
if (isNonNumeric || value === 0 ||
isUnitlessNumber.hasOwnProperty(name) && isUnitlessNumber[name]) {
return '' + value; // cast to string
if (typeof value === 'number' && value !== 0 &&
!(isUnitlessNumber.hasOwnProperty(name) && isUnitlessNumber[name])) {
return value+'px'; // Presumes implicit 'px' suffix for unitless numbers
}

if (typeof value === 'string') {
if (__DEV__) {
// Allow '0' to pass through without warning. 0 is already special and
// doesn't require units, so we don't need to warn about it.
if (component && value !== '0') {
var owner = component._currentElement._owner;
var ownerName = owner ? owner.getName() : null;
if (ownerName && !styleWarnings[ownerName]) {
styleWarnings[ownerName] = {};
}
var warned = false;
if (ownerName) {
var warnings = styleWarnings[ownerName];
warned = warnings[name];
if (!warned) {
warnings[name] = true;
}
}
if (!warned) {
warning(
false,
'a `%s` tag (owner: `%s`) was passed a numeric string value ' +
'for CSS property `%s` (value: `%s`) which will be treated ' +
'as a unitless number in a future version of React.',
component._currentElement.type,
ownerName || 'unknown',
name,
value
);
}
}
}
value = value.trim();
}
return value + 'px';
return ('' + value).trim();
}

module.exports = dangerousStyleValue;