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

Use 'class' and 'for' for DOM property names #1223

Closed
wants to merge 1 commit into from
Closed
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
2 changes: 1 addition & 1 deletion src/addons/ReactComponentWithPureRenderMixin.js
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ var shallowEqual = require('shallowEqual');
* mixins: [ReactComponentWithPureRenderMixin],
*
* render: function() {
* return <div className={this.props.className}>foo</div>;
* return <div class={this.props.class}>foo</div>;
* }
* });
*
Expand Down
2 changes: 1 addition & 1 deletion src/browser/ReactDOM.js
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ var objMapKeyVal = require('objMapKeyVal');
* valid according to `DOMProperty`.
*
* - Event listeners: `onClick`, `onMouseDown`, etc.
* - DOM properties: `className`, `name`, `title`, etc.
* - DOM properties: `class`, `name`, `title`, etc.
*
* The `style` property functions differently from the DOM API. It accepts an
* object mapping of style properties to values.
Expand Down
4 changes: 2 additions & 2 deletions src/browser/ui/__tests__/ReactMountDestruction-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -27,15 +27,15 @@ describe('ReactMount', function() {
document.documentElement.appendChild(mainContainerDiv);

var instanceOne = (
<div className="firstReactDiv">
<div class="firstReactDiv">
</div>
);
var firstRootDiv = document.createElement('div');
mainContainerDiv.appendChild(firstRootDiv);
React.renderComponent(instanceOne, firstRootDiv);

var instanceTwo = (
<div className="secondReactDiv">
<div class="secondReactDiv">
</div>
);
var secondRootDiv = document.createElement('div');
Expand Down
9 changes: 7 additions & 2 deletions src/browser/ui/dom/DOMProperty.js
Original file line number Diff line number Diff line change
Expand Up @@ -93,8 +93,13 @@ var DOMPropertyInjection = {

DOMProperty.getAttributeName[propName] = attributeName || lowerCased;

DOMProperty.getPropertyName[propName] =
DOMPropertyNames[propName] || propName;
var propertyName = DOMPropertyNames[propName];
if (propertyName) {
DOMProperty.getPossibleStandardName[propertyName.toLowerCase()] =
propName;
}

DOMProperty.getPropertyName[propName] = propertyName || propName;

var mutationMethod = DOMMutationMethods[propName];
if (mutationMethod) {
Expand Down
2 changes: 1 addition & 1 deletion src/browser/ui/dom/DOMPropertyOperations.js
Original file line number Diff line number Diff line change
Expand Up @@ -162,7 +162,7 @@ var DOMPropertyOperations = {
var propName = DOMProperty.getPropertyName[name];
var defaultValue = DOMProperty.getDefaultValueForProperty(
node.nodeName,
name
propName
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

(A bug that no one noticed before.)

Copy link
Member

Choose a reason for hiding this comment

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

Can we fix this independently? Also a new test? I'm not convinced yet that we'll take the rest.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I suppose so. #1225

);
if (!DOMProperty.hasSideEffects[name] ||
node[propName] !== defaultValue) {
Expand Down
8 changes: 4 additions & 4 deletions src/browser/ui/dom/DefaultDOMPropertyConfig.js
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ var DefaultDOMPropertyConfig = {
cellSpacing: null,
charSet: MUST_USE_ATTRIBUTE,
checked: MUST_USE_PROPERTY | HAS_BOOLEAN_VALUE,
className: MUST_USE_PROPERTY,
class: MUST_USE_PROPERTY,
cols: MUST_USE_ATTRIBUTE | HAS_POSITIVE_NUMERIC_VALUE,
colSpan: null,
content: null,
Expand All @@ -68,14 +68,14 @@ var DefaultDOMPropertyConfig = {
download: null,
draggable: null,
encType: null,
for: null,
form: MUST_USE_ATTRIBUTE,
formNoValidate: HAS_BOOLEAN_VALUE,
frameBorder: MUST_USE_ATTRIBUTE,
height: MUST_USE_ATTRIBUTE,
hidden: MUST_USE_ATTRIBUTE | HAS_BOOLEAN_VALUE,
href: null,
hrefLang: null,
htmlFor: null,
httpEquiv: null,
icon: null,
id: MUST_USE_PROPERTY,
Expand Down Expand Up @@ -165,10 +165,8 @@ var DefaultDOMPropertyConfig = {
y: MUST_USE_ATTRIBUTE
},
DOMAttributeNames: {
className: 'class',
gradientTransform: 'gradientTransform',
gradientUnits: 'gradientUnits',
htmlFor: 'for',
spreadMethod: 'spreadMethod',
stopColor: 'stop-color',
stopOpacity: 'stop-opacity',
Expand All @@ -182,7 +180,9 @@ var DefaultDOMPropertyConfig = {
autoCorrect: 'autocorrect',
autoFocus: 'autofocus',
autoPlay: 'autoplay',
class: 'className',
encType: 'enctype',
for: 'htmlFor',
hrefLang: 'hreflang',
radioGroup: 'radiogroup',
spellCheck: 'spellcheck',
Expand Down
12 changes: 6 additions & 6 deletions src/browser/ui/dom/__tests__/DOMPropertyOperations-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -72,14 +72,14 @@ describe('DOMPropertyOperations', function() {
expect(console.warn.argsForCall[0][0]).toContain('tabIndex');
});

it('should warn about class', function() {
it('should warn about className', function() {
spyOn(console, 'warn');
expect(DOMPropertyOperations.createMarkupForProperty(
'class',
'className',
'muffins'
)).toBe(null);
expect(console.warn.argsForCall.length).toBe(1);
expect(console.warn.argsForCall[0][0]).toContain('className');
expect(console.warn.argsForCall[0][0]).toContain('you mean class?');
});

it('should create markup for boolean properties', function() {
Expand Down Expand Up @@ -174,17 +174,17 @@ describe('DOMPropertyOperations', function() {
expect(foobarSetter.mock.calls[0][1]).toBe('cows say moo');
});

it('should set className to empty string instead of null', function() {
it('should set class to empty string instead of null', function() {
DOMPropertyOperations.setValueForProperty(
stubNode,
'className',
'class',
'selected'
);
expect(stubNode.className).toBe('selected');

DOMPropertyOperations.setValueForProperty(
stubNode,
'className',
'class',
null
);
// className should be '', not 'null' or null (which becomes 'null' in
Expand Down
2 changes: 1 addition & 1 deletion src/browser/ui/dom/__tests__/Danger-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ describe('Danger', function() {
});

it('should render markup with props', function() {
var markup = (<div className="foo" />).mountComponent(
var markup = (<div class="foo" />).mountComponent(
'.rX',
transaction,
0
Expand Down
4 changes: 2 additions & 2 deletions src/core/ReactPropTransferer.js
Original file line number Diff line number Diff line change
Expand Up @@ -51,9 +51,9 @@ var TransferStrategies = {
*/
children: emptyFunction,
/**
* Transfer the `className` prop by merging them.
* Transfer the `class` prop by merging them.
*/
className: createTransferStrategy(joinClasses),
class: createTransferStrategy(joinClasses),
/**
* Never transfer the `key` prop.
*/
Expand Down
2 changes: 1 addition & 1 deletion src/core/__tests__/ReactCompositeComponent-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ describe('ReactCompositeComponent', function() {
render: function() {
var className = cx({'anchorClass': this.props.anchorClassOn});
return this.props.renderAnchor ?
<a ref="anch" className={className}></a> :
<a ref="anch" class={className}></a> :
<b></b>;
}
});
Expand Down
20 changes: 10 additions & 10 deletions src/core/__tests__/ReactDOM-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -79,35 +79,35 @@ describe('ReactDOM', function() {
it("should purge the DOM cache when removing nodes", function() {
var myDiv = ReactTestUtils.renderIntoDocument(
<div>{{
theDog: <div className="dog" />,
theBird: <div className="bird" />
theDog: <div class="dog" />,
theBird: <div class="bird" />
}}</div>
);
// Warm the cache with theDog
myDiv.setProps({
children: {
theDog: <div className="dogbeforedelete" />,
theBird: <div className="bird" />
theDog: <div class="dogbeforedelete" />,
theBird: <div class="bird" />
}
});
// Remove theDog - this should purge the cache
myDiv.setProps({
children: {
theBird: <div className="bird" />
theBird: <div class="bird" />
}
});
// Now, put theDog back. It's now a different DOM node.
myDiv.setProps({
children: {
theDog: <div className="dog" />,
theBird: <div className="bird" />
theDog: <div class="dog" />,
theBird: <div class="bird" />
}
});
// Change the className of theDog. It will use the same element
// Change the class of theDog. It will use the same element
myDiv.setProps({
children: {
theDog: <div className="bigdog" />,
theBird: <div className="bird" />
theDog: <div class="bigdog" />,
theBird: <div class="bird" />
}
});
var root = ReactMount.getNode(myDiv._rootNodeID);
Expand Down
18 changes: 9 additions & 9 deletions src/core/__tests__/ReactDOMComponent-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -38,14 +38,14 @@ describe('ReactDOMComponent', function() {
transaction = new ReactReconcileTransaction();
});

it("should handle className", function() {
it("should handle class", function() {
var stub = ReactTestUtils.renderIntoDocument(<div style={{}} />);

stub.receiveComponent({props: { className: 'foo' }}, transaction);
stub.receiveComponent({props: { class: 'foo' }}, transaction);
expect(stub.getDOMNode().className).toEqual('foo');
stub.receiveComponent({props: { className: 'bar' }}, transaction);
stub.receiveComponent({props: { class: 'bar' }}, transaction);
expect(stub.getDOMNode().className).toEqual('bar');
stub.receiveComponent({props: { className: null }}, transaction);
stub.receiveComponent({props: { class: null }}, transaction);
expect(stub.getDOMNode().className).toEqual('');
});

Expand Down Expand Up @@ -120,7 +120,7 @@ describe('ReactDOMComponent', function() {
});

it("should remove properties", function() {
var stub = ReactTestUtils.renderIntoDocument(<div className='monkey' />);
var stub = ReactTestUtils.renderIntoDocument(<div class='monkey' />);

expect(stub.getDOMNode().className).toEqual('monkey');
stub.receiveComponent({props: {}}, transaction);
Expand Down Expand Up @@ -247,10 +247,10 @@ describe('ReactDOMComponent', function() {
});
});

it("should generate the correct markup with className", function() {
expect(genMarkup({ className: 'a' })).toHaveAttribute('class', 'a');
expect(genMarkup({ className: 'a b' })).toHaveAttribute('class', 'a b');
expect(genMarkup({ className: '' })).toHaveAttribute('class', '');
it("should generate the correct markup with class", function() {
expect(genMarkup({ class: 'a' })).toHaveAttribute('class', 'a');
expect(genMarkup({ class: 'a b' })).toHaveAttribute('class', 'a b');
expect(genMarkup({ class: '' })).toHaveAttribute('class', '');
});
});

Expand Down
8 changes: 4 additions & 4 deletions src/core/__tests__/ReactPropTransferer-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ describe('ReactPropTransferer', function() {
render: function() {
return this.transferPropsTo(
<input
className="textinput"
class="textinput"
style={{display: 'block'}}
type="text"
value=""
Expand All @@ -54,7 +54,7 @@ describe('ReactPropTransferer', function() {
.expectRenderedChild()
.toBeComponentOfType(React.DOM.input)
.scalarPropsEqual({
className: 'textinput',
class: 'textinput',
style: {display: 'block'},
type: 'text',
value: ''
Expand All @@ -74,7 +74,7 @@ describe('ReactPropTransferer', function() {
it('should transfer using merge strategies', function() {
var instance =
<TestComponent
className="hidden_elem"
class="hidden_elem"
style={{width: '100%'}}
/>;
ReactTestUtils.renderIntoDocument(instance);
Expand All @@ -83,7 +83,7 @@ describe('ReactPropTransferer', function() {
.expectRenderedChild()
.toBeComponentOfType(React.DOM.input)
.scalarPropsEqual({
className: 'textinput hidden_elem',
class: 'textinput hidden_elem',
style: {
display: 'block',
width: '100%'
Expand Down
10 changes: 5 additions & 5 deletions src/core/__tests__/refs-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -45,14 +45,14 @@ var ClickCounter = React.createClass({
for (i=0; i < this.state.count; i++) {
children.push(
<div
className="clickLogDiv"
class="clickLogDiv"
key={"clickLog" + i}
ref={"clickLog" + i}
/>
);
}
return (
<span className="clickIncrementer" onClick={this.handleClick}>
<span class="clickIncrementer" onClick={this.handleClick}>
{children}
</span>
);
Expand Down Expand Up @@ -187,15 +187,15 @@ describe('ref swapping', function() {
return (
<div>
<div
className="first"
class="first"
ref={count % 3 === 0 ? 'hopRef' : 'divOneRef'}
/>
<div
className="second"
class="second"
ref={count % 3 === 1 ? 'hopRef' : 'divTwoRef'}
/>
<div
className="third"
class="third"
ref={count % 3 === 2 ? 'hopRef' : 'divThreeRef'}
/>
</div>
Expand Down
4 changes: 2 additions & 2 deletions src/test/ReactTestUtils.js
Original file line number Diff line number Diff line change
Expand Up @@ -120,12 +120,12 @@ var ReactTestUtils = {

/**
* Finds all instance of components in the rendered tree that are DOM
* components with the class name matching `className`.
* components with the class name matching `class`.
* @return an array of all the matches.
*/
scryRenderedDOMComponentsWithClass: function(root, className) {
return ReactTestUtils.findAllInRenderedTree(root, function(inst) {
var instClassName = inst.props.className;
var instClassName = inst.props.class;
return ReactTestUtils.isDOMComponent(inst) && (
instClassName &&
(' ' + instClassName + ' ').indexOf(' ' + className + ' ') !== -1
Expand Down
Loading