Add method, tests, and docs for html method in ReactWrapper

[nfpiche]

Hope I was on the right track, this is for #31

[andersonba]

+1

[lelandrichardson]

how safe is this? if I understand correctly, this is assuming that the data-reactid attribute will be the last attribute of the node... how sure are we that this will be the case? A more robust regex might be good... or perhaps another approach to get this html?

[nfpiche]

My understanding is that data-reactid will always be appended by react at the end. I can definitely dig into it a little more to make sure I'm correct. I agree a more robust regex might be better in the long run though, in addition to another test case where data-reactid is somewhere else in the HTML.

Thanks for the feedback! I should have an updated pull request sometime tonight.

[lelandrichardson]

This looks great other than my one comment. Thanks for contributing! I apologize for not getting to this sooner. I've been in the midst of a hackathon the past couple of days and haven't had much chance to look at this.

[nfpiche]

Hmm, not sure why that change would decrease the coverage, even .01%!

[lelandrichardson]

@nfpiche we've been having issues with the coveralls integration. It's producing some strange results... i wouldn't worry about it.

Regarding your change... i'm a bit worried about capturing data attributes other than data-reactid. It seems like there could be some reasonable react code that someone would add in data-react-foo attributes for compatibility with some other JS on the page, and they would want to see it in the rendered output.

I'm thinking we should limit it to just data-reactid for now?

[lelandrichardson]

also, is the last + needed in this regex?

[nfpiche]

Yep good catch, not sure why I had that.

[nfpiche]

Yeah, changing it to catch just data-reactid is no problem.

I'm not 100% sure how to write a good test that show's it will delete the attribute we want from anywhere and leave the rest though. I tried adding my own data-reactid to a random place in the tag but react was obviously not thrilled about it :)

If you have a suggestion for a good way to test it I can implement it, but as it stands currently I've just been testing my regex here http://rubular.com/ and it seems to work as expected.

[nfpiche]

Oh now I see you prefer rebase over merge, not really familiar with doing a rebase, should I open a new pull request or did I rectify the rebase/merge difference in the next commit? Sorry for the confusion

[ljharb]

@nfpiche please don't open a new PR - where "source" is a remote pointing to airbnb/enzyme, and assuming "origin" is a remote pointing to your fork: git checkout add-html-to-render && git fetch source && git rebase source/master and if all is well, git push -u -f origin add-html-to-render is the simplest rebase. you can add a -i to the rebase command to get an interactive prompt in your $EDITOR, which is a bit more complicated but allows you to combine commits.

[nfpiche]

went through those commands, hope this fixes it! sorry again for the confusion

[vesln]

awesome work!

[lelandrichardson]

@nfpiche this looks ready to merge now. Do you mind squashing your commits and rebasing so we can keep master's history clean?

[ljharb]

(no need to squash down to one commit necessarily tho, just to a minimal set of conceptually atomic commits with meaningful commit messages :-) )

[blainekasten]

This video is a good tutorial on squashing commits. https://asciinema.org/a/11269

[nfpiche]

I think this isn't terrible this time

Thanks for all the help everyone

[ljharb]

if this is returning an actual dom node, would it make more sense to .cloneNode() it, and then removeAttribute('data-reactid'), and then .outerHTML?

[lelandrichardson]

you'd have to remove data-reactid from EVERY node.

[ljharb]

Array.prototype.forEach.call(findDomNode(n).querySelectorAll('[data-react-id]'), function (node) {
  node.removeAttribute('data-reactid');
});

seems like it'd be fine