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

Get outer DOM node from composite component wrapper #679

Conversation

christian-schulze
Copy link
Contributor

@christian-schulze christian-schulze commented Nov 13, 2016

As outlined in #623 and #446, implement ReactWrapper#getDOMNode which provides access to the outer most DOMComponent of a mounted composite component.

If you are happy for this to exist, I will add api documentation.

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 good overall, assuming other collabs are 👍 on adding the feature

it('should return the outer most DOMComponent of the wrapper', () => {
const wrapper = mount(<Test />);
expect(wrapper.getDOMNode()).to.have.property('className', 'outer');
});
Copy link
Member

Choose a reason for hiding this comment

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

let's also add a test that grabs the inner DOM node, so we have one test for "root", one for "not root", and one for "throws on multiples"

it('should return the outer most DOMComponent of the root wrapper', () => {
const wrapper = mount(<Test />);
expect(wrapper.getDOMNode()).to.have.property('className', 'outer');
});
Copy link
Member

Choose a reason for hiding this comment

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

nbd but let's make sure there's newlines between all the its :-)

@christian-schulze
Copy link
Contributor Author

One of travis CI processes failed relating to karma on the previous build, looked like some sort of timeout. Fingers crossed for this build.

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.

LGTM; could use a rebase down to one commit.

Will wait to merge until other collabs approve,

@ljharb
Copy link
Member

ljharb commented Nov 13, 2016

This also fixes #290

@nfcampos
Copy link
Collaborator

According to the React documentation findDOMNode can't be used on functional components, so this method cannot either. Two points:

  1. Should we add a method that works only for class components?
  2. If we do add it we at least need a note in the documentation stating it can't be used on functional components (and probably throw an explicit error if people try to do it).

@ljharb
Copy link
Member

ljharb commented Nov 15, 2016

Yes, i think we should add it as long as it throws when used on an SFC.

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 ensure that it throws when wrapping an SFC.

return this.single('getDOMNode', findDOMNode);
return this.single('getDOMNode', (n) => {
if (isFunctionalComponent(n)) {
throw new Error('Method “getDOMNode” cannot be used on functional components.');
Copy link
Member

Choose a reason for hiding this comment

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

This should probably be a TypeError

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.

LGTM; could still use a rebase down to a single commit

@christian-schulze christian-schulze force-pushed the get-outer-dom-node-from-composite-component branch from 830b56e to 27db1b0 Compare November 16, 2016 01:36
@christian-schulze
Copy link
Contributor Author

I just learnt how to rebase/squash 👍

@ljharb
Copy link
Member

ljharb commented Nov 16, 2016

@christian-schulze awesome! it still looks like it's not rebased on latest master tho, fwiw.

@christian-schulze christian-schulze force-pushed the get-outer-dom-node-from-composite-component branch from 27db1b0 to e466a7f Compare November 16, 2016 07:02
@christian-schulze
Copy link
Contributor Author

@ljharb rebased from latest master

@ljharb
Copy link
Member

ljharb commented Nov 16, 2016

@nfcampos et al; feel free to merge when you're ready

@christian-schulze christian-schulze force-pushed the get-outer-dom-node-from-composite-component branch from e466a7f to 0d263bd Compare November 18, 2016 09:49
@nfcampos
Copy link
Collaborator

@christian-schulze would you mind rebasing before I merge this?

@christian-schulze christian-schulze force-pushed the get-outer-dom-node-from-composite-component branch from 0d263bd to fd2939b Compare November 26, 2016 23:47
@christian-schulze
Copy link
Contributor Author

No worries @nfcampos, rebased

@nfcampos
Copy link
Collaborator

Thanks @christian-schulze! will merge as soon as build passes on travis :)

@christian-schulze
Copy link
Contributor Author

@nfcampos Build is green ✅

@nfcampos nfcampos merged commit fa68236 into enzymejs:master Nov 27, 2016
@nfcampos
Copy link
Collaborator

Merged, thanks!

@ljharb ljharb removed the question label Nov 27, 2016
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.

3 participants