Skip to content

[Fix] {ShallowWrapper,ReactWrapper}#{type,props}() should work with null nodes#162

Merged
lelandrichardson merged 2 commits intomasterfrom
shallow_fix_nulls_in_sfcs
Feb 4, 2016
Merged

[Fix] {ShallowWrapper,ReactWrapper}#{type,props}() should work with null nodes#162
lelandrichardson merged 2 commits intomasterfrom
shallow_fix_nulls_in_sfcs

Conversation

@ljharb
Copy link
Copy Markdown
Member

@ljharb ljharb commented Feb 3, 2016

…er#props()` should work with `null` nodes.

Fixes #113.
Comment thread src/ReactWrapper.js Outdated
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

i think in the Utils file there is a propsOfNode function that we should probably be using instead of this. It deals with things like null being passed in.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Sounds good - will that work the same in shallow and mount?

@ljharb ljharb force-pushed the shallow_fix_nulls_in_sfcs branch from c7d0670 to 198db6c Compare February 3, 2016 05:45
lelandrichardson added a commit that referenced this pull request Feb 4, 2016
[Fix] `{ShallowWrapper,ReactWrapper}#{type,props}()` should work with `null` nodes
@lelandrichardson lelandrichardson merged commit 3c06ba1 into master Feb 4, 2016
@lelandrichardson lelandrichardson deleted the shallow_fix_nulls_in_sfcs branch February 4, 2016 01:11
@jedwards1211
Copy link
Copy Markdown

@ljharb I'm wondering -- why would we want wrappers with null nodes to get passed to findWhere anyway? I'm assuming there are still other methods that won't work with null nodes (I stumbled on this problem with .text()). Wouldn't it be more robust to simply not pass wrappers with null nodes?

@ljharb
Copy link
Copy Markdown
Member Author

ljharb commented Feb 4, 2016

That's a good point, and may be an alternative approach.

@jedwards1211
Copy link
Copy Markdown

Yeah I was going to post an issue about .text() in .findWhere() but I could just post an issue about null nodes instead

@ljharb
Copy link
Copy Markdown
Member Author

ljharb commented Feb 4, 2016

Let's do that - findWhere and filterWhere etc perhaps shouldn't return null nodes? It's worth a discussion.

@jedwards1211
Copy link
Copy Markdown

Okay, sounds good

@jedwards1211
Copy link
Copy Markdown

And thanks for this awesome project!

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