ckirkendall / kioo

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

Improper tag nesting #35

Closed scttnlsn closed 9 years ago

scttnlsn commented 9 years ago

I was playing around with the Om example and changed part of main.html from:

<li class="nav-item"><a href="#">Placeholder</a></li>

to

<li class="nav-item">
    <a href="#">
        <h1>Placeholder</h1>
    </a>
</li>

Additionally, I changed the nav item snippet from:

(defsnippet my-nav-item "main.html" [:.nav-item]
  [[caption func]]
  {[:a] (do-> (content caption)
              (listen :onClick #(func caption)))})

to

(defsnippet my-nav-item "main.html" [:.nav-item]
  [[caption func]]
  {[:a] (listen :onClick #(func caption))
   [:h1] (content caption)})

The resulting HTML is:

<li class="nav-item" data-reactid=".0.0.1.0">
    <a href="#" data-reactid=".0.0.1.0.0"></a>
    <h1 data-reactid=".0.0.1.0.1">home</h1>
</li>

Why doesn't the h1 tag remain nested inside the a tag? I'm compiling against the lastest Kioo version from master (https://github.com/ckirkendall/kioo/tree/c5cf99cb9ddcf6758d6daaeb68621457dd8cc942).

ckirkendall commented 9 years ago

I was able to reproduce this. I don't know what is happening yet but I will take a look today.

ckirkendall commented 9 years ago

Well I think this is being caused by our html minifier. I will look into a work around later this week.

scttnlsn commented 9 years ago

@ckirkendall Have you made any progress with this? I dug into it a bit and was left under the impression that this might actually be an issue with Enlive.

Here's the result of using Enlive's html-resource function:

(html-resource "main.html")
({:type :dtd, :data ["html" nil nil]}
 {:tag :html,
  :attrs {:lang "en"},
  :content
  ("\n"
   {:tag :body,
    :attrs nil,
    :content
    ("\n"
     {:tag :header,
      :attrs nil,
      :content
      ("\n    "
       {:tag :h1, :attrs nil, :content ("Header placeholder")}
       "\n    "
       {:tag :ul,
        :attrs {:id "navigation"},
        :content
        ("\n        "
         {:tag :li,
          :attrs {:class "nav-item"},
          :content
          ("\n            "
           {:tag :a,
            :attrs {:href "#"},
            :content ("\n                ")}
           {:tag :h1, :attrs nil, :content ("Placeholder")}
           "\n            \n        ")}
         "\n    ")}
       "\n")}
     "\n"
     {:tag :div, :attrs {:class "content"}, :content ("place holder")}
     "\n")}
   "\n\n")})

The a and h1 tags are not nested as they should be.

scttnlsn commented 9 years ago

Yeah, looks like this is a known issue w/ Enlive's use of tagsoup (https://github.com/cgrand/enlive/issues/110). Perhaps we can swap out the Enlive parser to use Hickory or jsoup.

ckirkendall commented 9 years ago

That is ugly! I am a bit shocked this is just coming to light now. I assumed it was the minifier and was trying to reproduce with that. Thanks for figuring this out. I will take a look at the options and see if we can't swap out the base enlive parser. I do believe enlive supports the ability to have different parsers.

Creighton

On Mon, Oct 20, 2014 at 11:29 PM, Scott Nelson notifications@github.com wrote:

Yeah, looks like this is a known issue w/ Enlive's use of tagsoup ( cgrand/enlive#110 https://github.com/cgrand/enlive/issues/110). Perhaps we can swap out the Enlive parser to use Hickory or jsoup.

— Reply to this email directly or view it on GitHub https://github.com/ckirkendall/kioo/issues/35#issuecomment-59874169.

scttnlsn commented 9 years ago

I was able to get this working by using the JSoup parser. Here's a quick and dirty fix: https://github.com/scttnlsn/kioo/commit/26e8a82d69fc1a44b1ee34b318b924803c85523a Minor differences in the way whitespace was handled led to that nasty conditional. Perhaps there's a nicer way to deal with this.

scttnlsn commented 9 years ago

Looks like enlive-ws is also using tagsoup. I think that will also need to be changed.

scttnlsn commented 9 years ago

When running the Kioo tests against the updated enlive-ws (https://github.com/scttnlsn/enlive-ws/commit/21f2e148e62bd9cef37241fdd695b66379f1a187) there were a bunch of failures because the attributes were ordered slightly differently. Fixed here: https://github.com/scttnlsn/kioo/commit/4a732dfacee14f93023a22fcfbdd15025e8ab3ed

Everything is working for me as expected now.

ckirkendall commented 9 years ago

I decided to move the enlive-ws stuff inside kioo because I need to control this for both standard and minified html options. Thanks for diagnosing the issue and for fixing all the tests. You rock!

scttnlsn commented 9 years ago

Okay, great. So glad this is working now!

jessesherlock commented 9 years ago

I'll second that thanks, that was some great work! Also happy to have enlive-ws functions moved into Kioo as it's a tiny bit of code so that makes sense.