Skip to content

Conversation

@syranide
Copy link
Contributor

Fixes #1185.

#873

Putting this PR here for discussion, if we should keep tagName in overloaded DOM components or not.

I chose to reference the initial tagName rather than redefine, compresses better and keeps them nicely in sync.

   raw     gz Compared to last run
     =      = build/JSXTransformer.js
  +167    +36 build/react-with-addons.js
  +108    +15 build/react-with-addons.min.js
  +167    +37 build/react.js
  +108    +18 build/react.min.js

@vjeux
Copy link
Contributor

vjeux commented Jan 13, 2014

Do you know why github diff is completely broken?

@sophiebits
Copy link
Collaborator

Line endings change maybe?

@syranide
Copy link
Contributor Author

@vjeux Yes gah, installed something that messed up my git line ending settings, one sec I'll fix it.

@syranide
Copy link
Contributor Author

@vjeux Fixed

@spicyj You have no idea how completely node and everything else just broke down because it had changed from LF to CRLF (that's why I couldn't "grunt build", etc).

@vjeux
Copy link
Contributor

vjeux commented Jan 13, 2014

Nice, I like this diff

@sophiebits
Copy link
Collaborator

@syranide Can you rebase this and also add tagName to createFullPageComponent? It'll fix #1185.

@sophiebits
Copy link
Collaborator

Looks like this won't work if you rebase to a commit after descriptors. We can either do like tagName: button.type.prototype.tagName to copy the tag name over or we can add in ReactDOM.js:

diff --git a/src/browser/ReactDOM.js b/src/browser/ReactDOM.js
index 5cd3b35..2d6d9b7 100644
--- a/src/browser/ReactDOM.js
+++ b/src/browser/ReactDOM.js
@@ -47,6 +47,7 @@ function createDOMComponentClass(omitClose, tag) {
   Constructor.prototype = new ReactDOMComponent(tag, omitClose);
   Constructor.prototype.constructor = Constructor;
   Constructor.displayName = tag;
+  Constructor.tagName = tag;

   var ConvenienceConstructor = ReactDescriptor.createFactory(Constructor);

and then do button.type.tagName.

@sebmarkbage Do you have a preference between these?

@sebmarkbage
Copy link
Collaborator

This will mess with the debugger that lists these twice. This seems wrong since these are not real DOM nodes it can mess with all kind of feature detection.

The internal tests that used this are fixed by now. It uses a descriptor.type === React.DOM.img.type which is the preferred dynamic type testing strategy. tagName should be considered internal and renamed to _tagName.

Let's just make ReactTestUtils.Simulate.x more permissive and let it be called on anything that can resolve getDOMNode.

@zpao
Copy link
Member

zpao commented Apr 24, 2014

Let's do what @sebmarkbage said. I filed 1445.

@zpao zpao closed this Apr 24, 2014
@syranide syranide deleted the tagname branch May 7, 2014 22:18
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.

ReactTestUtils.Simulate.click does not work with jsdom

5 participants