Skip to content

Conversation

@syranide
Copy link
Contributor

Shouldn't be there right?

@sophiebits
Copy link
Collaborator

@vjeux said it needs to be there for some internal FB tests. I'd think those should be changed too; otherwise we should add tagName to all the wrappers for consistency.

@vjeux
Copy link
Contributor

vjeux commented Jan 13, 2014

ReactTestUtils are assuming that tagName is defined on all the DOM properties.

isDOMComponent: function(inst) {

scryRenderedDOMComponentsWithTag: function(root, tagName) {

findRenderedDOMComponentWithTag: function(root, tagName) {

I feel like we should add tagName to all the DOM tags instead of removing them

@vjeux vjeux closed this Jan 13, 2014
@sophiebits
Copy link
Collaborator

Well, isDOMComponent should probably be false if it's a composite component.

@syranide
Copy link
Contributor Author

@vjeux But can't ReactDOMComponent do that globally then? Oh wait, I thought they were extending ReactDOMComponent but they're extending ReactCompositeComponent, yeah this is sound.

@vjeux
Copy link
Contributor

vjeux commented Jan 13, 2014

We use those test util methods in a handful of places internally and they rely on the fact that tagName is defined. I don't see any downside of defining tagName for React.DOM components.

@syranide
Copy link
Contributor Author

@vjeux I don't know enough about the internals, but couldn't these components just extend or mixin ReactDOMComponent instead?

@syranide syranide deleted the noimgtag branch January 13, 2014 17:16
@vjeux
Copy link
Contributor

vjeux commented Jan 13, 2014

@spicyj ^^

@sophiebits
Copy link
Collaborator

I mean, they behave essentially like the composite component wrappers that they are, so that sounds sketchy to me. I can look a little more in depth later.

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.

3 participants