ckirkendall / kioo

Enlive/Enfocus style templating for Facebook's React and Om in ClojureScript.
Eclipse Public License 1.0
403 stars 39 forks source link

pre-parse html to prevent extra spans #11

Closed ghost closed 10 years ago

ghost commented 10 years ago

I was having some problems with css styling due to extra spans in the html.

I think these spans are generated by the html-resource function of enlive which preserves the formatting of the html. This causes "\n " string to be wrapped in spans by react.

This pull request is probably not perfect (i'm havn't used enlive before and written more clojure then java) but provides a way to pre-process the html so a lot of spans are prevented while not messing the html up to bad.

What are your thoughts on this subject?

swannodette commented 10 years ago

Just to clarify, Om doesn't create extra spans. However React will create extra spans if you give it something it doesn't know how to render.

ckirkendall commented 10 years ago

@swannodette yes I am referring to the om namespace in kioo. kioo.core tries real hard not to create extra spans by making sure to use apply where possible and flattening node structures to keep the arrays from creating extra spans. I have not figured out how to get around this in Om.

ghost commented 10 years ago

Thank you both for responding so fast.

@ckirkendall I hope I understand your last comment right, but most of the extra spans kioo generates in my case are caused by line-breaks in template html which are preserved by html-resource as strings.

for instance a kioo component template with:

<div>
     <span>Text</span>
</div>

has the following generated html:

<div data-reactid=".r[2rru9].[2]"><span data-reactid=".r[2rru9].[2].[0]">
  </span><span data-reactid=".r[2rru9].[2].[1]">Text</span><span data-reactid=".r[2rru9].[2].[2]">
</span></div>

Here the first and the last span are generated because of line-breaks in the template

when generating using OM's

(dom/div nil (dom/span nil "Text"))
<div data-reactid=".r[5r0pd].[2]"><span data-reactid=".r[5r0pd].[2].[0]">Text</span></div>

or a kioo component template without line-breaks

<div><span>Text</span></div>
<div data-reactid=".r[2bzum].[2]"><span data-reactid=".r[2bzum].[2].[0]">Text</span></div>

Is it possible that the line breaks generate an equavilent to om's:

(dom/div nil "\n      " (dom/span nil "Text") "\n        ")

Do you think it's worth fixing in kioo (in which case I might try to make a fix on emitting of the tags as you suggested) or do you think this will also be resolved by a possible react fix?

ckirkendall commented 10 years ago

I will certainly give it a look. React is working on this issue but if we can solve it in kioo we will. I will take a look at the enlive output and see what's up. My guess is we are getting a collection of strings when we span a line return. We might be able to detect strings and do some form of condense during the compile phase.

CK

On Thu, Feb 13, 2014 at 5:01 PM, Rolf Berthel notifications@github.comwrote:

Thank you both for responding so fast.

@ckirkendall https://github.com/ckirkendall I hope I understand your last comment right, but most of the extra spans kioo generates in my case are caused by line-breaks in template html which are preserved by html-resource as strings.

for instance a kioo component template with:

Text

has the following generated html:

Text

Here the first and the last span are generated because of line-breaks in the template

when generating using OM's

(dom/div nil (dom/span nil "Text"))

Text

or a kioo component template without line-breaks

Text
Text

Is it possible that the line breaks generate an equavilent to om's:

(dom/div nil "\n " (dom/span nil "Text") "\n ")

Do you think it's worth fixing in kioo (in which case I might try to make a fix on emitting of the tags as you suggested) or do you think this will also be resolved by a possible react fix?

Reply to this email directly or view it on GitHubhttps://github.com/ckirkendall/kioo/pull/11#issuecomment-35031471 .

ckirkendall commented 10 years ago

You are correct that it generates.

(dom/div nil "\n      " (dom/span nil "Text") "\n        ")

While I was able to remove it with some pre-compile work, I don't think removing it is a good idea. The problem is this whitespace is significant and because of the way react works these types of whitespace get surrounded by spans. Trimming out the line can cause issues. Take for instance the html below:

<div>
<span>test</span>
<span>test2</span>
</div>

This would display as "test test". If we trimmed the line returns we get "testtest". I think we may have to punt on this one and let the react people figure out how not to surround strings by spans when in a list with other nodes.

ckirkendall commented 10 years ago

If you want to follow the status on how react is trying to fix this problem see here: https://github.com/facebook/react/pull/742

ghost commented 10 years ago

Thanks for the extra information. I think your right. Waiting for a change in react is better.

travis commented 10 years ago

Hey, just found this thread after noticing this issue in my own code. I'm using kioo 0.3.0 which uses om 0.5.0, which seems to be using react 0.9.0. Given the release date for 0.9.0 I expected it to contain the fix you mentioned in facebook/react#742, which was merged in January, but I'm definitely still seeing lots of extra spans from linebreaks in my HTML templates. Is there more work that needs to be done on the Kioo side or is this unexpected?

The particularly problematic case of this bug, for me, is when I render tables - lots of extra spans are being added and my error log is littered with stuff like "Danger: Discarding unexpected node: <span data-reactid=​".0.5.3.1.1.0.0">​​​""

I'm getting around this by killing all linebreaks in my table markup, but that makes my templates pretty darn ugly.

ckirkendall commented 10 years ago

Travis, A lot of consolidation of spans was done with om 0.9.0 and the current version of react but there are still many cases that are unavoidable given how react works. I do not think Kioo can do any more than it already does to consolidate them. If you look back in the history of the om unit tests you will see how bad it was before react fixed several items.

Creighton

On Thu, Apr 3, 2014 at 12:46 PM, Travis Vachon notifications@github.comwrote:

Hey, just found this thread after noticing this issue in my own code. I'm using kioo 0.3.0 which uses om 0.5.0, which seems to be using react 0.9.0. Given the release date for 0.9.0 I expected it to contain the fix you mentioned in facebook/react#742https://github.com/facebook/react/pull/742, which was merged in January, but I'm definitely still seeing lots of extra spans from linebreaks in my HTML templates. Is there more work that needs to be done on the Kioo side or is this unexpected?

Reply to this email directly or view it on GitHubhttps://github.com/ckirkendall/kioo/pull/11#issuecomment-39475046 .

travis commented 10 years ago

Ah, I see, thanks. I'll take a look at what Kioo is doing right now to consolidate this stuff - it would be great if there were some way to clean this up in the case of tables, as the errors I'm getting are pretty ugly, but I'm happy to wrap my head around that and try to come up with a fix. Thanks for the pointers!

ckirkendall commented 10 years ago

Travis, If you can put together an example of the issues you are seeing I can certainly take a look at what we might be able to do.

CK

On Thu, Apr 3, 2014 at 3:49 PM, Travis Vachon notifications@github.comwrote:

Ah, I see, thanks. I'll take a look at what Kioo is doing right now to consolidate this stuff - it would be great if there were some way to clean this up in the case of tables, as the errors I'm getting are pretty ugly, but I'm happy to wrap my head around that and try to come up with a fix. Thanks for the pointers!

Reply to this email directly or view it on GitHubhttps://github.com/ckirkendall/kioo/pull/11#issuecomment-39496451 .