bad `ngStyle` example

[zoechi]

https://angular.io/docs/ts/latest/guide/template-syntax.html#!#ngStyle

setStyles() {
  let styles = {
    // CSS property names
    'font-style':  this.canSave      ? 'italic' : 'normal',  // italic
    'font-weight': !this.isUnchanged ? 'bold'   : 'normal',  // normal
    'font-size':   this.isSpecial    ? '24px'   : '8px',     // 24px
  };
  return styles;
}

should IMHO be considered as DON'T example

• It binds a method in the view [ngClass]="setClasses()" which is discouraged and should be avoided except one knows what she's doing

• It returns a new instance for every call. If it would at least cache the result in an instance field and return that as long as not dependencies are changed, it would be ok.

[wKoza]

@zoechi

except one knows what she's doing

That's not the case here ? What are you thinking about ?

[zoechi]

except one knows what she's doing

I mean that demonstrating binding to methods in docs without special mention of the pitfalls is a bit dangerous because people might not know about the pitfalls.
If you know that the method must not return a new instance every time it's called, and that the method shouldn't do expensive work, then binding to a method is just fine.
Binding to a getter is basically the same but I think it's known more widely that getters shouldn't do expensive work.

[wardbell]

Do you really think someone is going to treat this as a recommended practice. The point is to show how the NgClass works syntactically.

I'd happily look at a PR. I'm worried that, in making it "correct", we'll lose sight of the point. If you can keep it simple, I'm interested.

[zoechi]

Do you really think someone is going to treat this as a recommended practice.

That's why I created this issue, because someone was arguing on SO that this should work because it is even explained this way at (with the link in my original issue comment).

Changing it to

styles = {
    // CSS property names
    'font-style':  this.canSave      ? 'italic' : 'normal',  // italic
    'font-weight': !this.isUnchanged ? 'bold'   : 'normal',  // normal
    'font-size':   this.isSpecial    ? '24px'   : '8px',     // 24px
};
setStyles() {
  return styles;
}

would already fix the main issue but then

styles = {
    // CSS property names
    'font-style':  this.canSave      ? 'italic' : 'normal',  // italic
    'font-weight': !this.isUnchanged ? 'bold'   : 'normal',  // normal
    'font-size':   this.isSpecial    ? '24px'   : '8px',     // 24px
};

with

<div [ngStyle]="styles">

would also do.

I'm not sure what of this is part of the point that is supposed to be made by this example.

[ph55]

Do you really think someone is going to treat this as a recommended practice.

Well, here I am.

Whole discussion started from stackoverflow question
In case of rendering multiple components of the same type, setStyles() would be called on each of them when change detection runs.

From docs:

The NgStyle directive may be the BETTER CHOICE when we want to set many inline styles at the same time.

As you see, its not just

show how the NgClass works syntactically

Its actually proposed as preferred option without explaining pitfalls.

[wKoza]

Ok well, I prefer the second option

    styles = {
        // CSS property names
        'font-style':  this.canSave      ? 'italic' : 'normal',  // italic
        'font-weight': !this.isUnchanged ? 'bold'   : 'normal',  // normal
        'font-size':   this.isSpecial    ? '24px'   : '8px',     // 24px
    };

with

    <div [ngStyle]="styles">

We can understand how NgStyle directive works syntactically.

[wardbell]

Ya all nailed me for being a grumpy old man. I'll get out a PR. Thanks for making the case.

[zoechi]

No worries 😄, ❤️ your work!

[wardbell]

Fixing in PR #3076