slim-template / slim

Slim is a template language whose goal is to reduce the syntax to the essential parts without becoming cryptic.
https://slim-template.github.io
MIT License
5.31k stars 501 forks source link

Attribute hash #109

Closed minad closed 12 years ago

minad commented 13 years ago

All attribute assignments must be static currently. It is not possible to render dynamic attribute names. This could be useful for data-* attributes.

Proposal: = data = {:a => 1, :b => 2} p %data class="test" Text

renders to

Text

stonean commented 13 years ago

Would rather not restricted dynamic attributes to data-*, how about something like:

p @ = { 'data-a' => 1, 'data-b' => 2}

or

p @ = generate_attributes
stonean commented 13 years ago

Another potential solution:

p id="notes" .= generate_attributes

I thought the .= could be read as "the result of the method goes here". If that result is a hash, we create an attributes/value for each key/value.

Since the meaning of .= is pretty generic, we may be able to reuse the syntax in other areas.

thoughts?

yury commented 13 years ago

What about empty squared braces []=

p [] = {data-a:1, data-v:2}

to mimic css attribute selectors

p [] = generate_attributes
stonean commented 13 years ago

@minad @fredwu any thoughts on this?

gferraz commented 13 years ago

What about this syntax?

    - attributes = {selected: true, class: fine}
    div(attributes) = content
    div(generate_attributes)

I known the current syntax for attributes is also a valid ruby expression (attr = val), so it might be undesirably ambiguous. Anyway...

yury commented 13 years ago

I propose to drop [] ({} and () is more than enough) grouping syntax for "attr=value" attributes and use it for hash attributes:

 # prototype for dynamic_attributes method
 def dynamic_attributes attributes={}
   current_node.attributes.merge! attributes
 end

and pass to this method anything between [] :

div[selected: true, class:'fine'] # we pass dynamic_attributes(selected: true, class:'fine')

Example with var:

- generated_attributes = {id: 'some_id', another:'bla'}
div[generated_attributes] # we pass dynamic_attributes(generated_attributes)

Also we can call #to_json on values in that hash to allow following:

div#user[data: {name: 'Yury', avatar:'http://blabla.jpg' }] 
# outputs <div data="{'name':'Yury', 'avatar':'http://blabla.jpg'}"></div>

Then with jquery we can access name and avatar with data method:

alert($('#user').data().name);
stonean commented 13 years ago

That's a good proposal. As a side note, all other attributes use the html syntax and think we should stick with it for consistency. i.e:

selected = true 

versus

selected: true

I think for true values, the result should just be the key.

div[itemscope=true]

should result in

<div itemscope>

fyi, itemscope is from the microdata format

yury commented 13 years ago

@stonean, The main idea that everything between [] is valid ruby code that returns Hash. I used ruby 1.9 syntax, but it also valid to use =>

# ruby < 1.9
div[:id => 'bla'] # maps to dynamic_attributes(:id => 'bla')
# or with ruby >= 1.9
div[id: 'bla'] # maps to dynamic_attributes(id: 'bla')
# or complex one
div[one_hash.merge(another_hash).merge(id: 'bla')] 
# maps to dynamic_attributes(one_hash.merge(another_hash).merge(id: 'bla'))

As for microdata we can use nil instead true. Not so clean, but true may be used for something more useful.

div[itemscope:nil] or div[:itemscope => nil]
stonean commented 13 years ago

I understand but that is inconsistent with the other markup and would rather people have an expectation of consistency if possible. Maybe this is an exception, I will think about it. Gotta get back to the day job. :(

Thanks for the feedback, much appreciated!

yury commented 13 years ago

Ok, after reading some docs about Boolean Attributes using nil seems to me bad idea. What about using ? at the end of attribute to mimic boolean nature.

We have several choices for syntax:

1.

  div itemscope? content

2.

div itemscope?=true content

3. Combined

stonean commented 13 years ago

@minad what do you think about treating [] differently?

stonean commented 13 years ago

Will spend some time focusing on this after #129 is complete

minad commented 13 years ago

I think we should solve this issue after 1.0

stonean commented 13 years ago

I agree. Nothing has really felt right yet.

minad commented 13 years ago

What about treating {} differently? Would be similar to ruby hash syntax then:

in 1.9

a{href: '...'} link
a{ruby_code_that_returns_hash} link

in 1.8

a{:href=> '...'} link
a{ruby_code_that_returns_hash} link

The problem is backward compatibility :(

yury commented 13 years ago

I wrote about [] to mimic ruby Hash[]. And implementation could use Hash[] to handle args. And [] are similar to css attributes selectors.

minad commented 13 years ago

Hmm, I like {} more because it is exactly the ruby syntax. But good idea to use Hash[].

gferraz commented 13 years ago

+1 for {} syntax

yury commented 13 years ago

No, {} is not exactly ruby syntax: {ruby_code_that_returns_hash} looks strange for ruby syntax.

minad commented 13 years ago

yes, you're right :)

But for "normal hashes" it looks like it, so I think it is more intuitive

yury commented 13 years ago

And one more thing

"Normal hashes" a{href: '...'} link - is less common use case, because it can be written as a href='…' link More common use case for dynamic attributes is to pass hash var - a{@attributes} link And in this case [] is more natural choice (a[@attributes] link). Again:

  1. It is very close to slim css selectors nature (witch I like a lot)
  2. It is very close and understandable as we pass args to Hash[]
gferraz commented 13 years ago
yury commented 13 years ago

@gferraz,

I think slim is more about css selectors and that is very cool. Ok. If slim will use {} for dynamic attributes and you will write code like this because you like it:

a{href:'/', target='_blank'} Home
div{class:'nice', id:'some_id'}
input{type:'hidden', value:''}

You will get render performance lost because all attributes will be dynamic. If slim choose [] you still can write things like:

div[@dynamic_attributes]{your='static-attributes'} Content
minad commented 13 years ago

@yury: Chaining is currently not possible as in your example

div[@dynamic_attributes]{your='static-attributes'} Content

I don't think it should ever be possible. Even if there are static attributes, there is some post-processing necessary if all-dynamic attributes are given: Attribute merging, removing duplicates etc. Example:

.test[:class => [:dynamic_class1, :dynamic_class2]]

class="test dynamic_class1 dynamic_class" should be the result, not class="test" class="dynamic_class1 dynamic_class"

yury commented 13 years ago

@minad: That's sad, because I really need this functionality.

Merging is required only for class (join by ' ') attribute and may be for id (join by '_'). But for the rest of attributes merging logic is undefined.

Handle class and id attributes is ok. But other attributes like href, target, width, height, style, alt, title can be pure static. If current implementation doesn't generate static Temple for these attributes then this is a place to get more rendering speed.

This is example from my app, where I need dynamic attributes:

 .card data-url=place_path(place) data-id=place.id =place.name

I what to extract data-url and data-id to helper:

 #places_helper.rb
 def place_meta
   {'data-url'=>place_path(place), 'data-id'=>place.id}
 end

 #and then _card.html.slim
 .card[place_meta place] =place.name
minad commented 13 years ago

If attribute merging is not enabled for an attribute, the attribute is overwritten by subsequent assignments. If we would implement the optimization, duplicate attributes could be generated. Is it valid XML/HTML if you have attribute duplicates?

stonean commented 13 years ago

The HTML4 DTD ( http://www.w3.org/TR/html4/sgml/dtd.html ) doesn't state attributes can be duplicated. I did some simple tests in Chrome and Firefox and both appear to honor the first attribute; subsequent attributes of the same name are ignored.

Just ran the HTML Validator against my test and it errors on elements with multiple attributes of the same name.

gferraz commented 13 years ago

@yuri: Thanks for pointing out the performance penalty when using dynamic atributes without thinking (I'm a such newbie :-|). Nonetheless, there is a legitimate case, I believe: When we have fixed attributes and dynamic values, one might want to make the hash in the template (not in a helper) to achieve more clarity: ``` a{href: @my_url, :target: @user.preferences. link_target}

which would be (a little bit) aesthetically better than:
``` a{{href: @my_url, :target: @user.preferences. link_target}}

or

yury commented 13 years ago

I don't think it is valid, but browsers take first value and all works fine http://jsfiddle.net/kVFmD/

It will be rare enough use case. Moreover slim allows to generate invalid attributes on elements and it is ok. For instance:

 a custom-href='invalid attribute'

Slim will output invalid HTML5 code. Developer still need to run external XML/HTML validators to make sure output is correct. So attributes duplicates are ok for good speed up.

I don't think that developers rely on slim overwrite policy and write duplicate attributes. I think, slim masks developers errors. And if duplicates are made by mistake it will hard to find error with slim overwrite policy but easy to find them in result output.

About optimization: It is easy to detect static attributes even with class attribute and make them static. For instance:

 a.nice href="/" class="red" Go Home
  1. It doesn't have dynamic attributes (no [] or {})
  2. class value is string literal (no interpolation)

We can produce static output for a tag

That was overthinked example. Most elements in my code just use attributes that don't need merging:

a.nice.red href="/" Go Home
  1. No dynamic attributes ([] or {})
  2. No attributes to merge

So we output pure static content.

Just looked though my views and found a lot of pure static elements. If slim will implement that kind of optimization it will be very speedy in production.

yury commented 13 years ago

@gferraz, turns out that I was wrong and all attributes in slim are dynamic right now. So no performance lost here %), but speed can be improved.

As for syntax it will be:

a{href: @my_url, target:@user.preferences.link_target}

or

a[href:@my_url, target:@user.preferences.link_target]

@minad, @stonean, If slim is trying to merge attributes every time now it would be very interesting to see benchmarks with attributes in template. As I can see, current benchmark doesn't use attributes.

minad commented 13 years ago

@yury

  1. Not all attributes are dynamic in slim. Attributes in quotes are static and don't generate any overhead.
  2. Slim tries to merge only attributes which are set in

    Temple::HTML::Fast.set_default_options :attrdelimiter => {'id' => '', 'class' => ' '}

Only id and class are merged.

minad commented 13 years ago

@yury: Just checked, Temple doesn't overwrite attributes. It throws an error e.g. "Multiple href attributes specified". This check is dones at compile time. For dynamic attributes this has to be done at runtime.

yury commented 13 years ago

@minad, ok, just to make whole picture Elements like a#home.nice.link href='/' Link are static right now? Or id and class attributes are dynamic?

As for chaining, logic can be very simple: Suppose we have following template

-@dynamic_attributes = {src:'/path/to/img.png'} # defined in controller
img.avatar[@dynamic_attributes] width='100' height='100' src='path/to/default.png' title='Avatar'

To avoid duplicates we can simple collect static attributes at compile time and extract them from @dynamic attributes at runtime. At render time we will remove src key from @dynamic_attributes, because we already have it in static. If user prefer src value from @dynamic_attributes he can use merge or reverse_merge in rails.

-@dynamic_attributes = {src:'/path/to/img.png'} # defined in controller
img.avatar[@dynamic_attributes.reverese_merge src:'path/to/default.png'] width='100' height='100' title='Avatar'

So user controls overwrite logic and it is looks ok. Of course, all of that should be done if we detect dynamic_attributes on element.

minad commented 13 years ago

Code generated by Slim (check with bin/slimrb -c). As you see it is static.

a#home.nice.link href='/' Link

_buf = []; _buf << ("<a class=\"nice link\" href=\"/\" id=\"home\">Link"\
"</a>"); _buf = _buf.join
yury commented 13 years ago

Thats cool. Thanks for slimrb %)

So what about chaining logic?

minad commented 13 years ago

Who of the current users needs this feature?

minad commented 13 years ago

seems not - reopen if needed

yury commented 13 years ago

Emm.... I still need this feature.

stonean commented 13 years ago

I have recently seen this use case in some Haml code. This should be open until a solution is delivered.

minad commented 13 years ago

I don't think we have to duplicate every haml feature. Who else needs this? You can always generate arbitrary html code using =

stonean commented 13 years ago

This isn't about Haml, it's about functionality. If anything I would like to improve the Haml syntax if possible.

I don't think you're going to get a lot of people replying saying they need it. Based on downloads alone a lot more people use Slim than watch the repo.

I agree there are ways around it, but the point is simplicity and cleanliness. If we can come up with a better way we should do it, I don't want to dismiss it.

minad commented 13 years ago

As I said, you can always emulate this by using your own helper method. So this is only syntactic sugar and I would also try to keep Slim as simple as possible.

minad commented 13 years ago

But I am not completly against this feature, I proposed it ;)

stonean commented 13 years ago

I know. :)

I just don't want us to close the issue. Keeping it open is a good reminder to revisit the problem. The complexity comes in keeping Slim's simplicity while providing the functionality. We could have hacked some horrendous code just to get it to work, but we aren't going to force fit a solution. Nobody wants that.

coolstorybro commented 12 years ago

Not having this feature is kind of a dealbreaker. I'd love to use Slim instead of HAML, but it's not worth losing this feature.

stonean commented 12 years ago

@coolstorybro if you have some real world use cases for dynamic attribute names that would help me. I just haven't had the need hence no solid solution yet.

coolstorybro commented 12 years ago

@stonean What about helper methods that return an attribute hash which may or may not have values? e.g.,

def helper_method_foo if something {:attr1 => foo, :attr2 => bar} else {:attr3 => wibble} end end

In HAML, you'd just do %element#id.class1.class2{helper_method_foo}. Am I mistaken and there's already a way to do this in Slim? I thought this was what the topic was about. This pattern has been used in the last three projects I've been on, and I use it in almost all my personal projects.

An example of where this is commonly used (in my experience) is when rendering an index page and you want to show different styles or attach different attributes depending upon the item's status, e.g.,

%li.foo.bar{li_attrs_for(object)}

where li_attrs_for could return anything from additional CSS classes (highlight records in a certain state) to a custom id attribute based on the object's database id. It's really handy.

I don't have time to fork Slim and add support for it, so I'll have to stick with HAML for now.

jacquescrocker commented 12 years ago

+1 on this. best feature of haml is passing in hashes. I can have helpers that return stuff like

%li{add_class("active", :if => check_current?(:home))}  Home
%li{add_class("active", :if => check_current?(:contact))}  Contact

Not sure how the syntax would look in Slim.

13k commented 12 years ago

+1 on this too. My use case for this is to be able to write more generic partials that can accept custom HTML attributes from the caller, so I could do this (with a supposed hash attributes syntax):

partial

- article_opts ||= {}
article[article_opts]
  p = article.content

caller1

render 'partial', locals: { article: @article, article_opts: { data-editable: true } }

caller2

render 'partial', locals: { article: @article }
stonean commented 12 years ago

We've been using Slim a lot more a work lately and this issue is popping up more frequently. This means I've been putting a lot more thought into it.

Here are a couple things:

1) I really don't like using {} or [] differently, it promotes code like (the above comment):

- article_opts ||= {}

I think setting having code like this in your views is a bad practice. I've seen the confusion it creates and it's not good.

2) As mentioned above, you can create helper (presenter) code to generate the tag, but I don't think that's ideal as it moves all template code into the helper. This results in a markup mix of confusion especially when the tags that need this functionality are structurally important.

We've had a lot of suggestions and earlier I suggested a syntax like:

p @ = { 'data-a' => 1, 'data-b' => 2}

p @ = data_attributes

However, that really didn't stick, but I'm beginning to settle on something very similar:

p @data_attributes

Where data_attributes is a method call that returns a hash whose key/values are added as attributes.

\ I haven't dug into the implementation details so some of the following could be changed.

Similar to '#' and '.' shortcuts, this could allow us to:

/ add attr_hash to the div
#myid .myclass @attr_hash

/ create a div with the attributes returned by attr_hash
@attr_hash

For normal tag usage:

a href='/blah' @attr_hash My Title

input type='submit' value='Submit' id='formSubmit' @data_attributes

Some of you may find confusion with the Ruby instance variable syntax, but ultimately it's about Slim and I think the @ symbol suits the meaning most appropriately.

Regarding dynamic attributes, I don't think Slim should try to check your code for setting an attribute twice or appending to an existing attribute. For example:

/ The home_link_attributes method returns { 'class' => 'bluelink', 'data-variable' => '3334'}
a id='myid' class='myclass' @home_link_attributes

If we can easily/efficiently check for class to append the values, then fine, but Slim's not responsible for correcting code. I think it's entirely acceptable for the result to be:

<a id='"myid" class="myclass" class="bluelink" data-variable="3334">