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 content property #120

Merged
merged 1 commit into from
Jun 28, 2013
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 0 additions & 1 deletion src/core/ReactDOMIDOperations.js
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,6 @@ var invariant = require('invariant');
* @private
*/
var INVALID_PROPERTY_ERRORS = {
content: '`content` must be set using `updateTextContentByID()`.',
dangerouslySetInnerHTML:
'`dangerouslySetInnerHTML` must be set using `updateInnerHTMLByID()`.',
style: '`style` must be set using `updateStylesByID()`.'
Expand Down
33 changes: 8 additions & 25 deletions src/core/ReactNativeComponent.js
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,6 @@ var registrationNames = ReactEventEmitter.registrationNames;
// For quickly matching children type, to test if can be treated as content.
var CONTENT_TYPES = {'string': true, 'number': true};

var CONTENT = keyOf({content: null});
var DANGEROUSLY_SET_INNER_HTML = keyOf({dangerouslySetInnerHTML: null});
var STYLE = keyOf({style: null});

Expand All @@ -50,14 +49,10 @@ function assertValidProps(props) {
if (!props) {
return;
}
// Note the use of `!=` which checks for null or undefined.
var hasChildren = props.children != null ? 1 : 0;
var hasContent = props.content != null ? 1 : 0;
var hasInnerHTML = props.dangerouslySetInnerHTML != null ? 1 : 0;
// Note the use of `==` which checks for null or undefined.
invariant(
hasChildren + hasContent + hasInnerHTML <= 1,
'Can only set one of `children`, `props.content`, or ' +
'`props.dangerouslySetInnerHTML`.'
props.children == null || props.dangerouslySetInnerHTML == null,
'Can only set one of `children` or `props.dangerouslySetInnerHTML`.'
);
invariant(
props.style == null || typeof props.style === 'object',
Expand Down Expand Up @@ -156,7 +151,7 @@ ReactNativeComponent.Mixin = {
return innerHTML.__html;
}
} else {
var contentToUse = this.props.content != null ? this.props.content :
var contentToUse =
CONTENT_TYPES[typeof this.props.children] ? this.props.children : null;
var childrenToUse = contentToUse != null ? null : this.props.children;
if (contentToUse != null) {
Expand Down Expand Up @@ -219,8 +214,8 @@ ReactNativeComponent.Mixin = {
styleUpdates[styleName] = '';
}
}
} else if (propKey === DANGEROUSLY_SET_INNER_HTML ||
propKey === CONTENT) {
} else if (propKey === DANGEROUSLY_SET_INNER_HTML) {
// http://jsperf.com/emptying-speed
ReactComponent.DOMIDOperations.updateTextContentByID(
this._rootNodeID,
''
Expand Down Expand Up @@ -274,11 +269,6 @@ ReactNativeComponent.Mixin = {
nextProp
);
}
} else if (propKey === CONTENT) {
ReactComponent.DOMIDOperations.updateTextContentByID(
this._rootNodeID,
'' + nextProp
);
} else if (registrationNames[propKey]) {
putListener(this._rootNodeID, propKey, nextProp);
} else {
Expand All @@ -305,17 +295,10 @@ ReactNativeComponent.Mixin = {
* @param {ReactReconcileTransaction} transaction
*/
_updateDOMChildren: function(nextProps, transaction) {
Copy link
Member

Choose a reason for hiding this comment

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

I haven't thought this through, but it feels like this method can be simplified. I wouldn't worry about trying to though, just sharing my gut feeling.

var thisPropsContentType = typeof this.props.content;
var thisPropsContentEmpty =
this.props.content == null || thisPropsContentType === 'boolean';
var nextPropsContentType = typeof nextProps.content;
var nextPropsContentEmpty =
nextProps.content == null || nextPropsContentType === 'boolean';

var lastUsedContent = !thisPropsContentEmpty ? this.props.content :
var lastUsedContent =
CONTENT_TYPES[typeof this.props.children] ? this.props.children : null;

var contentToUse = !nextPropsContentEmpty ? nextProps.content :
var contentToUse =
CONTENT_TYPES[typeof nextProps.children] ? nextProps.children : null;

// Note the use of `!=` which checks for null or undefined.
Expand Down
4 changes: 2 additions & 2 deletions src/core/__tests__/ReactDOMIDOperations-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -33,8 +33,8 @@ describe('ReactDOMIDOperations', function() {
expect(function() {
ReactDOMIDOperations.updatePropertyByID(
'testID',
keyOf({content: null}),
'testContent'
keyOf({dangerouslySetInnerHTML: null}),
{__html: 'testContent'}
);
}).toThrow();

Expand Down
34 changes: 7 additions & 27 deletions src/core/__tests__/ReactMultiChildText-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -71,10 +71,6 @@ var assertSingleChild = function(instance, text) {
};

// Helpers
var renderSingleContentChild = function(text) {
var d = ReactTestUtils.renderIntoDocument(<div content={text} />);
return d;
};
var renderSingleTextChild = function(text) {
var d = ReactTestUtils.renderIntoDocument(<div>{text}</div>);
return d;
Expand Down Expand Up @@ -158,11 +154,6 @@ describe('ReactMultiChildText', function() {
assertMultiChild(d, 'hello', 'goodbye');
});

it('should render content to single text node', function() {
var d = renderSingleContentChild('hello');
assertNodeText(d, 'hello');
});

it('should render a single text child to a single text node', function() {
var d = renderSingleTextChild('hello');
assertNodeText(d, 'hello');
Expand Down Expand Up @@ -234,12 +225,6 @@ describe('ReactMultiChildText', function() {
assertNodeText(d, '0');
});

it('should render content number zero as text node', function() {
var d = renderSingleContentChild(0);
// false should act exactly as a null child
assertNodeText(d, '0');
});

it('should render zero string as string child', function() {
var d = renderMultipleTextChildren('0', 234.2);
// false should act exactly as a null child
Expand All @@ -250,23 +235,23 @@ describe('ReactMultiChildText', function() {
var d = renderMultipleTextChildren('0', 234.2);
// false should act exactly as a null child
assertMultiChild(d, '0', '234.2');
d.replaceProps({content: '0'});
d.replaceProps({children: '0'});
assertNodeText(d, '0');
});

it('should render zero number as string child then text node', function() {
var d = renderMultipleTextChildren(0, 234.2);
// false should act exactly as a null child
assertMultiChild(d, '0', '234.2');
d.replaceProps({content: 0});
d.replaceProps({children: 0});
// BELOW REVEALS A BUG IN JSDOM
// assertNodeText(d, '0'); // This works in the browser.
});

it('should render multiple children then switch to inline', function() {
var d = renderMultipleTextChildren('hello', 'goodbye');
assertMultiChild(d, 'hello', 'goodbye');
d.replaceProps({content: 'hello'});
d.replaceProps({children: 'hello'});
assertNodeText(d, 'hello');
});

Expand All @@ -286,13 +271,6 @@ describe('ReactMultiChildText', function() {
assertMultiChild(d, 'hello', 'goodbye');
});

it('should render content, then switch to text components ', function() {
var d = renderSingleContentChild('hello');
assertNodeText(d, 'hello');
d.replaceProps({children: ['hello', 'goodbye']});
assertMultiChild(d, 'hello', 'goodbye');
});

it('should render inline child, then switch to composite', function() {
var d = renderSingleTextChild('hello');
assertNodeText(d, 'hello');
Expand All @@ -302,9 +280,11 @@ describe('ReactMultiChildText', function() {
.toBeCompositeComponentWithType(TestCompositeComponent);
});

it('should throw if rendering both content and children', function() {
it('should throw if rendering both HTML and children', function() {
expect(function() {
ReactTestUtils.renderIntoDocument(<div content="asdf">ghjkl</div>);
ReactTestUtils.renderIntoDocument(
<div dangerouslySetInnerHTML={{_html: 'abcdef'}}>ghjkl</div>
);
}).toThrow();
});
});
18 changes: 2 additions & 16 deletions src/core/__tests__/ReactNativeComponent-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -292,25 +292,11 @@ describe('ReactNativeComponent', function() {
});

it("should validate against multiple children props", function() {
expect(function() {
mountComponent({ content: '', children: '' });
}).toThrow(
'Invariant Violation: Can only set one of `children`, ' +
'`props.content`, or `props.dangerouslySetInnerHTML`.'
);

expect(function() {
mountComponent({ content: '', dangerouslySetInnerHTML: '' });
}).toThrow(
'Invariant Violation: Can only set one of `children`, ' +
'`props.content`, or `props.dangerouslySetInnerHTML`.'
);

expect(function() {
mountComponent({ children: '', dangerouslySetInnerHTML: '' });
}).toThrow(
'Invariant Violation: Can only set one of `children`, ' +
'`props.content`, or `props.dangerouslySetInnerHTML`.'
'Invariant Violation: Can only set one of `children` or ' +
'`props.dangerouslySetInnerHTML`.'
);
});

Expand Down