Strip out white space text nodes#31
Conversation
|
Might this PR be unnecessary as #30 is already closed? |
aknuds1
left a comment
aknuds1
left a comment
There was a problem hiding this comment.
Would like input from others as to whether this whitespace removal is a good idea.
| assert.equal(reactHtml, htmlExpected); | ||
| }); | ||
|
|
||
| it('should not insert spans into tables even if there is white space', function() { |
There was a problem hiding this comment.
As far as I can tell, this test doesn't add anything except verify that whitespace text nodes are stripped.
| }); | ||
|
|
||
| // Should it strip out contents of a div containing only white space? | ||
| it('should strip out contents of a div containing only white space', function() { |
There was a problem hiding this comment.
Should we actually remove text nodes that are whitespace only? The comment seems to indicate the PR author isn't sure either.
| }); | ||
|
|
||
| // Should it strip out white space that is a sibling to an element? | ||
| it('should strip out white space that is a sibling to an element', function() { |
There was a problem hiding this comment.
Does this make any semantic difference compared to just stripping whitespace text nodes?
|
Could it be better to leave to some HTML tidying library to handle concerns such as unnecessary whitespace, which this PR addresses? |
|
Thanks @aknuds1 - I agree, this feels like too heavy a solution to the problem. I like your idea of leaving it to another library to deal with this. Is it a worry that the test in #30 passes but still produces a warning that a
doesn't produce a warning. |
3 participants
Fixes #30 ("Should not insert spans into tables even if there is white space") by stripping out all white-space text nodes.
This changes non-table nodes too, e.g.:
<div> </div>into<div></div><div> <span>hi</span>bye</div>into<div><span>hi</span>bye</div>.I'm not sure if that is desirable. If it's not, is there a better solution?