machty / emblem.js

Emblem.js - Ember-friendly, indented syntax alternative for Handlebars.js
http://emblemjs.com
MIT License
1.04k stars 81 forks source link

Added new attribute binding syntax from 1.11 #234

Closed thec0keman closed 9 years ago

thec0keman commented 9 years ago

This is a WIP on enabling the HTMLBars attribute syntax.

This makes a few changes to Emblem's DSL to facilitate this:

Existing colon syntax maps to inline if

div class=foo:bar:baz

converts to

<div class={{ if foo 'bar' 'baz' }}>

This syntax seemed important to retain to ensure backwards compatibility (not to mention significant refactoring). This wraps all of the values as strings as I believe this is how the bindAttr syntax worked.

This will work with mixing unbound and bound class names, as well as single mustache surrounding multiple colon expressions:

.alert class={ foo:bar:baz one:two:three }

converts to:

<div class="alert {{ if foo 'bar' 'baz'}} {{if one 'two' 'three'}}">

However, one important thing to note here is the HTMLBars syntax does not convert a true to the variable name, i.e.: {{ bind-attr class=foo }}.

For bindAttr, if foo===true then this generates <div class="foo">. However, for HTMLBars this would return <div class="true">. In these cases, users will need to alter the colon syntax slightly to retain the original functionality: class=foo:foo.

Inline if syntax for attributes is support by single mustache

class={ if foo bar 'baz' }

This allows for more proper HTMLBars syntax for attributes. This also allows for attribute assignment for both bound and unbound values. This syntax supports any possible string, so the following should work:

class={ if isGoogle 'http://www.google.com' }

There are several hacks to this so that existing single mustache expressions should still work. With this change, any attribute assignment with single mustache passes all of the contents of the mustache unchanged.

One current limitation which I think makes sense is to ignore this rule when inside a quote. In this case the user should return double mustache:

div style="margin-top: {{ model.xValue }}px;"

I've done a fair amount of initial testing and so far everything is looking good. This is my first attempt at PEGjs so I imagine the code could look a lot prettier. I've tested this against our largish Ember app and so far there are no significant issues. Other than the one issue with bindAttr -> HTMLBars syntax, this required no refactoring of the code.


Thoughts? Are these the best syntax changes? One thing that seemed a bit hackish was I did a bit of refactoring of how bound and unbound class attributes are parsed, yet it still depends on PEGjs prefixing unbound attributes with a :.

Bestra commented 9 years ago

I think the syntax looks reasonable and you've got some good tests in place for the results.

adamniedzielski commented 9 years ago

Will this pull request remove following deprecations thrown from Ember 1.13?

DEPRECATION: The `bind-attr` helper ('<project name>/templates/application.hbs' @ L1:C366) is deprecated in favor of HTMLBars-style bound attributes.

I identified the line which causes the deprecation and it is:

img src=session.currentUser.imgSrc
thec0keman commented 9 years ago

We've been using this in production now for almost 2 months and haven't run into any issues. Would it be possible to get this merged soon? (Especially since the bind-attr syntax has been deprecated for awhile and will be removed in 2.0.)

If there are concerns / suggestions on improving the approach I'd be more than happy to work on it this some more. This was my first attempt at using PEGjs so I'm sure it has room for improvement. Thanks! cc @bantic / @mixonic

machty commented 9 years ago

Thank you! Your pegjs chops seem good to me.

thec0keman commented 9 years ago

@machty would it be helpful to add a note about the colon syntax being converted to inline if in the changelog? I'm not sure how common the colon syntax is in templates, but it may be helpful to be aware of this transform. I can submit a merge request if you'd like.

machty commented 9 years ago

Yeah, I added something to the changelog but feel free to embellish if it's not enough and i'll merge.