weavejester / hiccup

Fast library for rendering HTML in Clojure
http://weavejester.github.io/hiccup
Eclipse Public License 1.0
2.68k stars 174 forks source link

Input tags rendered as input elements #63

Closed pauldorman closed 12 years ago

pauldorman commented 12 years ago

This patch allows input elements to be treated like any other tag in Hiccup, with the proviso that they have no content. For example an input of type "password" can be created with the familiar Hiccup form [:password#someid.someclass.otherclass {:value "foo"}].

If there is content the tag gets rendered like any other element. If it wasn't for the HTML "button" element (<button type="button">Hi, I'm a button</button>), which is also the name for an HTML input type (<input type="button" value="Hi, I'm a button too!" />) there would be some cheap opportunities for error handling, but there you go.

This patch does not displace the functions in the hiccup.form namespace (which are unaffected by this change), but may be a step towards deprecating the majority of the functions that create HTML input elements.

Apart from the few lines to implement the change and its tests, there was one change to replace the non-element <text /> with <br /> in the "empty tags" tests.

Those people keen to use all those fancy HTML5 input types will rejoice I'm sure! ;)

weavejester commented 12 years ago

This patch complicates the relation between Hiccup data structures and the HTML it produces. In general, additional functionality should be added through transforming the input structure, not by complecting the compiler.

pauldorman commented 12 years ago

That's a valid argument, though I think in practice it will simplify the task of creating form elements with Hiccup, including all those pesky new HTML 5 input types. The current functions for input fields also make the assumption that input fields require at least a "name" attribute, and possibly a "value" attribute, neither of which are required for valid HTML.

Though this change "complects" the compiler, it is extremely minor, is completely optional, and would broaden the choice for the programmer.

weavejester commented 12 years ago

You can achieve the same functionality by layering a transformation function on top of the data structure, so there's no need to put this in the Hiccup compiler directly.

Secondly, even though the code change is small, the complexity added is not minor by any means. Currently the Hiccup compiler has the following rules:

tag : keyword | symbol | string
element : [tag & body] | [tag map & body]
body : (element | string | list)*

You're proposing to add an additional rule:

input : [type & body] | [type attrs & body]

Effectively you're increasing the complexity of the system by 33% through the addition of this rule, even if the rule takes up only a minimum of code.

So why should we care about complexity if the code change is small? Because it makes it more difficult to read and transform the input data. If I write a transformation function that walks the Hiccup data structure and transforms it in some way, suddenly I need to account for a whole new rule.

In general, any additional complexity in a data model needs to be considered very carefully. Also, since this would break compatibility, this change would need to be a Hiccup 2.0 change, and Hiccup 2.0 will work slightly differently, in that it won't render directly to strings any more.

pauldorman commented 12 years ago

Actually, it's only [type & body], and I'm unaware of where this change breaks any compatibility. Given that this is such a minor change I'm happy to just close this pull request and just work from my own fork.

It would be useful if you could put up some clear guidelines on the wiki for people wanting to make contributions to Hiccup.

weavejester commented 12 years ago

It breaks compatibility because in the current version of Hiccup:

user> (html [:password {:value "foo"}])
"<password value=\"foo\">"

A better solution would be to place this functionality in a transformation function that walks the tree and transforms the input:

user> (transform-inputs [:password {:value "foo"}])
[:input {:type "password" :value "foo"}]

This will be easier in Hiccup 2.0, which will produce consistent data structures instead of strings, so there will be no need to transform the Hiccup vectors directly (which can be a little tricky because attribute maps are optional).

user> (hiccup2/html [:password {:value "foo"}])
{:tag "password" :attrs {:value "foo"} :content nil}

user> (-> (hiccup2/html [:password {:value "foo"}])
          (transform-inputs))
{:tag "input" :attrs {:type "password" :value "foo"} :content nil}

As for clear guidelines, there's nothing really beyond "write well-designed code". I think your core idea has promise, but the Hiccup compiler isn't the place for it. A transformation function would achieve exactly the same result, but with none of the disadvantages.

pauldorman commented 12 years ago

A good sleep has done me wonders and I am now happy to say that I now truly get where you're coming from. Thanks for taking the time to beat me down with common sense.