Add method, tests, and docs for html method in ReactWrapper #71
Conversation
|
+1 |
| * @returns {String} | ||
| */ | ||
| html() { | ||
| return this.single(n => findDOMNode(n).outerHTML.replace(/\sdata-reactid[^>]+/g, '')); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
|
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. |
|
Hmm, not sure why that change would decrease the coverage, even .01%! |
|
@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 I'm thinking we should limit it to just |
| * @returns {String} | ||
| */ | ||
| html() { | ||
| return this.single(n => findDOMNode(n).outerHTML.replace(/\sdata-react[-\w]+="[^"]+"+/g, '')); |
There was a problem hiding this comment.
also, is the last + needed in this regex?
There was a problem hiding this comment.
Yep good catch, not sure why I had that.
|
Yeah, changing it to catch just 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 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. |
|
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 |
|
@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: |
nfpiche
force-pushed
the
add-html-to-render
branch
from
3605e49
to
814c89c
Compare
|
went through those commands, hope this fixes it! sorry again for the confusion |
|
awesome work! |
|
@nfpiche this looks ready to merge now. Do you mind squashing your commits and rebasing so we can keep master's history clean? |
|
(no need to squash down to one commit necessarily tho, just to a minimal set of conceptually atomic commits with meaningful commit messages :-) ) |
|
This video is a good tutorial on squashing commits. https://asciinema.org/a/11269 |
nfpiche
force-pushed
the
add-html-to-render
branch
from
3bf0b5b
to
226393c
Compare
|
I think this isn't terrible this time Thanks for all the help everyone |
| * @returns {String} | ||
| */ | ||
| html() { | ||
| return this.single(n => findDOMNode(n).outerHTML.replace(/\sdata-reactid+="[^"]+"/g, '')); |
There was a problem hiding this comment.
if this is returning an actual dom node, would it make more sense to .cloneNode() it, and then removeAttribute('data-reactid'), and then .outerHTML?
There was a problem hiding this comment.
you'd have to remove data-reactid from EVERY node.
There was a problem hiding this comment.
Array.prototype.forEach.call(findDomNode(n).querySelectorAll('[data-react-id]'), function (node) {
node.removeAttribute('data-reactid');
});seems like it'd be fine
Add method, tests, and docs for html method in ReactWrapper
Hope I was on the right track, this is for #31