ember-animation / liquid-fire

Animations & transitions for ambitious Ember applications.
Other
1.15k stars 199 forks source link

Boolean class handling is broken for liquid helpers #230

Open yankeeinlondon opened 9 years ago

yankeeinlondon commented 9 years ago

Unless I've misinterpreted the instructions the liquid-if block component this should toggle the "show-navigation" class based on the the boolean showNavigation parameter:

<div id="nav-container">
  {{#liquid-if showNavigation class="show-navigation"}}
    <div id="navigation">navigation</div>
  {{else}}
    <!-- no menu -->
  {{/liquid-if}}
  <div id="main-content">{{yield}}</div>
</div>

What I get instead is proper switching between the if and else blocks based on the showNavigation parameter but the "show-navigation" class stays set regardless of state.

I am using the latest build on master and Ember 1.11.0.

ef4 commented 9 years ago

Thanks for the bug report. In the process of answering your question I doubled-checked the behavior and found a problem. Your example:

{{#liquid-if showNavigation class="show-navigation"}}

sets the class to a constant string, which is why it's always present. Whereas this:

{{#liquid-if showNavigation class=showNavigation}}

Is the normal way to bind to a variable instead. This is where I found the bug -- this doesn't do quite what you want. When showNavigation is boolean and true, it's supposed to result in a class like "show-navigation", but instead you get "inner-class", which is a name that's leaking out of the liquid-if implementation.

The easiest workaround is:

  {{#liquid-if showNavigation class=(if showNavigation "show-navigation") }}
yankeeinlondon commented 9 years ago

Work-around works great! Thanks @ef4 .

yankeeinlondon commented 9 years ago

BTW, the reason I adopted the original syntax I did is that the online example takes a similar approach:

{{#liquid-if isBike class="vehicles"}}
  <label>
    Would you like a helmet? {{input type="checkbox" checked="helmet"}}
  </label>
  // ...

Unless I'm still missing something -- and my current flu-like symptoms make that entirely plausible -- this should be updated too.

winne42 commented 4 years ago

@ef4 What is the current state of this bug, please?