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

feat: Added hostNodes method to ReactWrapper #1179

Merged
merged 1 commit into from
Oct 3, 2017
Merged

Conversation

FezVrasta
Copy link
Contributor

fixes #1174

Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

Looks great :-) can you please add it to ShallowWrapper as well, along with tests and docs?

@FezVrasta
Copy link
Contributor Author

Going to add some tests tomorrow!

</div>,
);
expect(wrapper.find('.foo').length).to.equal(2);
expect(wrapper.find('.foo').hostNodes().length).to.equal(1);
Copy link
Member

Choose a reason for hiding this comment

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

could we assert on the contents too, not just the length? something like:

const hostNodes = wrapper.find('.foo').hostNodes();
expect(hostNodes).to.have.lengthOf(1);
expect(hostNodes.is('div')).to.equal(true);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

😩 I was trying to perform the PR trough the GitHub UI but I guess I'll have to clone the project locally 😅

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done!

Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

Please remove the yarn.lock file; airbnb does not use yarn.

@FezVrasta
Copy link
Contributor Author

Sorry, it slipped in accidentally. Done.

Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

Pending a rebase (ideally down to one commit), this LGTM

@ljharb
Copy link
Member

ljharb commented Sep 29, 2017

(if you need help rebasing, i'm happy to take care of it)

@FezVrasta
Copy link
Contributor Author

I'm not sure if the rebase ended up well 🤔

@FezVrasta FezVrasta force-pushed the 1174 branch 2 times, most recently from b7efdca to 06de273 Compare September 29, 2017 17:36
@FezVrasta
Copy link
Contributor Author

Ok now it looks much better 😅

@ljharb
Copy link
Member

ljharb commented Sep 29, 2017

@FezVrasta still looks like there's a merge commit; would you like me to rebase it for you?

@FezVrasta
Copy link
Contributor Author

FezVrasta commented Sep 29, 2017

I don't see merge commits?
image

(btw, go ahead)

@ljharb
Copy link
Member

ljharb commented Sep 29, 2017

aha, now it's good :-)

@FezVrasta
Copy link
Contributor Author

Still, Travis doesn't look that happy... 🙄

</div>,
);
const hostNodes = wrapper.find('.foo').hostNodes();
expect(wrapper.find('.foo')).to.have.length(2);
Copy link
Member

Choose a reason for hiding this comment

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

maybe try lengthOf instead of length here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Other tests in the same page use .length, why so?

Copy link
Member

Choose a reason for hiding this comment

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

They should all be using lengthOf; it was just a thought about fixing the travis tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It doesn't seem to help

@ljharb
Copy link
Member

ljharb commented Oct 2, 2017

k, rebased, added some docs, and fixed the issue (it needs .filterWhere; .filter takes a string, not a function).

@ljharb
Copy link
Member

ljharb commented Oct 2, 2017

and finally, React 13 doesn't support SFCs; so the component in the tests needed to be class-based.

@ljharb ljharb merged commit 4fec9bb into enzymejs:master Oct 3, 2017
#### Examples

```jsx
const wrapper = mount(<div><MyComponent className="foo" /><div className="foo" /></div>);
Copy link
Contributor

@pfhayes pfhayes Oct 3, 2017

Choose a reason for hiding this comment

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

should this be shallow instead of mount?

Copy link
Member

Choose a reason for hiding this comment

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

Whoops, good catch! Want to submit a PR?

Copy link
Contributor

Choose a reason for hiding this comment

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

@TheSharpieOne
Copy link

This is kinda critical for migration from v2 to v3. Not trying to rush, but I don't want to monkey patch this... so any plans on releasing this sooner than later?

@ljharb
Copy link
Member

ljharb commented Oct 3, 2017

I think we're hoping to cut a release this week.

@lelandrichardson
Copy link
Collaborator

@TheSharpieOne enzyme v3.1.0 was just published. enjoy.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enzyme 3.0.0 "find()" queries not just DOM elements
5 participants