New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Using NgIf with NgFor throws errors #4792

Closed
brandyscarney opened this Issue Oct 16, 2015 · 17 comments

Comments

Projects
None yet
10 participants
@brandyscarney

brandyscarney commented Oct 16, 2015

When I add *ng-if to an element, which also has *ng-for, it is causing the variable to be set to null, even though the expression in the *ng-if is returning true. I am trying to only show the element if the expression returns true, and not show it at all otherwise.

Plunker reproducing the issue: http://plnkr.co/edit/ozJydSz1XqiNtrVCUfm1?p=preview

Is there a way I can do this?

@vicb

This comment has been minimized.

Show comment
Hide comment
@vicb

vicb Oct 16, 2015

Contributor

There could be only one template directive per element (* directive)

// current - invalid
      <div *ng-if="showCategory()" *ng-for="#category of categories">
        Category: {{ category.displayName }}
      </div>

// working - extra div level
      <div *ng-if="showCategory()">
        <div *ng-for="#category of categories">
          Category: {{ category.displayName }}
        </div>
      </div>

// working - single div
      <template [ng-if]="showCategory()">
        <div *ng-for="#category of categories">
          Category: {{ category.displayName }}
        </div>
      </template>

The error is in your example but we should report an error here - could you check if there is already a pending issue & close if this is the case ?

Contributor

vicb commented Oct 16, 2015

There could be only one template directive per element (* directive)

// current - invalid
      <div *ng-if="showCategory()" *ng-for="#category of categories">
        Category: {{ category.displayName }}
      </div>

// working - extra div level
      <div *ng-if="showCategory()">
        <div *ng-for="#category of categories">
          Category: {{ category.displayName }}
        </div>
      </div>

// working - single div
      <template [ng-if]="showCategory()">
        <div *ng-for="#category of categories">
          Category: {{ category.displayName }}
        </div>
      </template>

The error is in your example but we should report an error here - could you check if there is already a pending issue & close if this is the case ?

@thelgevold

This comment has been minimized.

Show comment
Hide comment
@thelgevold

thelgevold Oct 17, 2015

Contributor

What is the reasoning behind the limitation of one template directive per element?
This is not the case in Angular 1.x and it seems like an unnecessary limitation IMO. It's also an annoyance when converting from Angular 1.x to Angular 2.0 since you now have to change your markup to use a wrapper element.

Contributor

thelgevold commented Oct 17, 2015

What is the reasoning behind the limitation of one template directive per element?
This is not the case in Angular 1.x and it seems like an unnecessary limitation IMO. It's also an annoyance when converting from Angular 1.x to Angular 2.0 since you now have to change your markup to use a wrapper element.

@vicb

This comment has been minimized.

Show comment
Hide comment
@vicb

vicb Oct 17, 2015

Contributor

In this particular case, what exactly do you want to do if have 10 categories, but showCategories() returns false: evaluate the if ten times to display nothing in the end or evaluate the if first and do not call for ?

You probably want the second solution in this trivial case but to solve all the cases things have to be explicit.

Contributor

vicb commented Oct 17, 2015

In this particular case, what exactly do you want to do if have 10 categories, but showCategories() returns false: evaluate the if ten times to display nothing in the end or evaluate the if first and do not call for ?

You probably want the second solution in this trivial case but to solve all the cases things have to be explicit.

@thelgevold

This comment has been minimized.

Show comment
Hide comment
@thelgevold

thelgevold Oct 17, 2015

Contributor

One solution is to give directives priority to dictate evaluation order. In this case ng-if could take precedence over ng-for, so if ng-if === false the execution of the ng-for is skipped all together

Contributor

thelgevold commented Oct 17, 2015

One solution is to give directives priority to dictate evaluation order. In this case ng-if could take precedence over ng-for, so if ng-if === false the execution of the ng-for is skipped all together

@S-YOU

This comment has been minimized.

Show comment
Hide comment
@S-YOU

S-YOU Oct 17, 2015

What is the reasoning behind the limitation of one template directive per element?

May be they don't want to walk the DOM to collect directives like angular 1 does? perf?

S-YOU commented Oct 17, 2015

What is the reasoning behind the limitation of one template directive per element?

May be they don't want to walk the DOM to collect directives like angular 1 does? perf?

@vicb

This comment has been minimized.

Show comment
Hide comment
@vicb

vicb Oct 17, 2015

Contributor

priorities does not work with 3rd-party directives (they might have different need). Being explicit work all the time.

Perf are not the problem here (and will be much better than angular 1).

Contributor

vicb commented Oct 17, 2015

priorities does not work with 3rd-party directives (they might have different need). Being explicit work all the time.

Perf are not the problem here (and will be much better than angular 1).

@thelgevold

This comment has been minimized.

Show comment
Hide comment
@thelgevold

thelgevold Oct 17, 2015

Contributor

In the end it's not a big deal as long as people are aware of the limitation, but I don't see why an optional directive priority property with a default value would'n't work for custom directives. Priority could then be offset to make sure CORE_DIRECTIVES always take precedence.

Contributor

thelgevold commented Oct 17, 2015

In the end it's not a big deal as long as people are aware of the limitation, but I don't see why an optional directive priority property with a default value would'n't work for custom directives. Priority could then be offset to make sure CORE_DIRECTIVES always take precedence.

@alexpods

This comment has been minimized.

Show comment
Hide comment
@alexpods

alexpods Oct 17, 2015

I think, it would be very cool if directives are evaluated in the same order as they exist in the markup. For example:

<div *ng-for="#name of people; #isEven=even" *ng-if="isEven">
    {{ name }}
</div>

could be desugared into:

<template ng-for #name [ng-for-of]="people" #isEven="even">
  <template [ng-if]="isEven">
    <div>{{ name }}</div>  
  </template>
</template>   

while:

<div *ng-if="showNames" *ng-for="#name of people">
    {{ name }}
</div>

could be desugared into:

<template  [ng-if]="showNames">
  <template ng-for #name [ng-for-of]="people">
    <div>{{ name }}</div>  
  </template>
</template>   

Unfortunately, DOM NamedNodeList doesn't guarantee to maintain any order. But maybe it's one more reason to implement #4417.

What do you think? @vicb @thelgevold @mhevery @pkozlowski-opensource

alexpods commented Oct 17, 2015

I think, it would be very cool if directives are evaluated in the same order as they exist in the markup. For example:

<div *ng-for="#name of people; #isEven=even" *ng-if="isEven">
    {{ name }}
</div>

could be desugared into:

<template ng-for #name [ng-for-of]="people" #isEven="even">
  <template [ng-if]="isEven">
    <div>{{ name }}</div>  
  </template>
</template>   

while:

<div *ng-if="showNames" *ng-for="#name of people">
    {{ name }}
</div>

could be desugared into:

<template  [ng-if]="showNames">
  <template ng-for #name [ng-for-of]="people">
    <div>{{ name }}</div>  
  </template>
</template>   

Unfortunately, DOM NamedNodeList doesn't guarantee to maintain any order. But maybe it's one more reason to implement #4417.

What do you think? @vicb @thelgevold @mhevery @pkozlowski-opensource

@pkozlowski-opensource

This comment has been minimized.

Show comment
Hide comment
@pkozlowski-opensource

pkozlowski-opensource Oct 17, 2015

Member

Interesting.... This would be clear departure from HTML way of doing things (as mentioned, it doesn't guarantee order of attrs). But IMO this is a slippery path to take - can't put a finger on it, but my gut-feeling screams "no" atm :-)

I think I've got a bigger problem with the de-sugared form (there is no way I could remember / write it correctly from the top of my head...) so I agree that there is a pb to solve here.

Member

pkozlowski-opensource commented Oct 17, 2015

Interesting.... This would be clear departure from HTML way of doing things (as mentioned, it doesn't guarantee order of attrs). But IMO this is a slippery path to take - can't put a finger on it, but my gut-feeling screams "no" atm :-)

I think I've got a bigger problem with the de-sugared form (there is no way I could remember / write it correctly from the top of my head...) so I agree that there is a pb to solve here.

@S-YOU

This comment has been minimized.

Show comment
Hide comment
@S-YOU

S-YOU Oct 17, 2015

I think, it would be very cool if directives are evaluated in the same order as they exist in the markup. For example:

as far as I noticed, some versions of internet explorer re-order attributes alphabetically.

S-YOU commented Oct 17, 2015

I think, it would be very cool if directives are evaluated in the same order as they exist in the markup. For example:

as far as I noticed, some versions of internet explorer re-order attributes alphabetically.

@mhevery

This comment has been minimized.

Show comment
Hide comment
@mhevery

mhevery Oct 19, 2015

Member

We sort of supported this in AngularJS v1, and it created endless sets of issues. So we are not going to support this in Angular 2. There is easy work around to just nest these in a template tag.

Member

mhevery commented Oct 19, 2015

We sort of supported this in AngularJS v1, and it created endless sets of issues. So we are not going to support this in Angular 2. There is easy work around to just nest these in a template tag.

@brandyscarney

This comment has been minimized.

Show comment
Hide comment
@brandyscarney

brandyscarney Oct 19, 2015

Thanks for the responses! My plunker above was very simple to show the problem. My actual use case is a bit more complex. I need to use one of the properties of the category to show/hide the element.

Wrapping it in a template tag won't work for this as I won't have access to the category object. And I am actually using a button instead of a div, so wrapping the inner content with a NgIf will still show the button itself.

After reading the above replies, I decided to hide the button with a class. So hopefully I don't run into problems with this approach.

I created a new Plunker to show my more complex use case for anyone that stumbles on this: http://plnkr.co/edit/hht4aH7QXKbF15o3IuSI?p=preview

Let me know if there is a better way to do this, I am still learning Angular 2. :)

@vicb I created an issue for better error messages here: #4817

brandyscarney commented Oct 19, 2015

Thanks for the responses! My plunker above was very simple to show the problem. My actual use case is a bit more complex. I need to use one of the properties of the category to show/hide the element.

Wrapping it in a template tag won't work for this as I won't have access to the category object. And I am actually using a button instead of a div, so wrapping the inner content with a NgIf will still show the button itself.

After reading the above replies, I decided to hide the button with a class. So hopefully I don't run into problems with this approach.

I created a new Plunker to show my more complex use case for anyone that stumbles on this: http://plnkr.co/edit/hht4aH7QXKbF15o3IuSI?p=preview

Let me know if there is a better way to do this, I am still learning Angular 2. :)

@vicb I created an issue for better error messages here: #4817

@vicb

This comment has been minimized.

Show comment
Hide comment
@vicb

vicb Oct 19, 2015

Contributor

@brandyscarney thanks for creating a new issue.

You are probably looking at something like:

      <button *ng-for="#category of categoriesByName('communication')">
          Multi Category: {{ category.displayName }}
      </button>

categoriesByName() would be a method on your class returning a list of categories filtered by Name.

There is one important thing: you must not return a new array each time the method is called otherwise the change detection will get confused (ie return categories.filter(c => c.name === name) will not work). You should only return a new array when categories have been modified. There are plans to improve this by implementing a trackBy functionality.

Contributor

vicb commented Oct 19, 2015

@brandyscarney thanks for creating a new issue.

You are probably looking at something like:

      <button *ng-for="#category of categoriesByName('communication')">
          Multi Category: {{ category.displayName }}
      </button>

categoriesByName() would be a method on your class returning a list of categories filtered by Name.

There is one important thing: you must not return a new array each time the method is called otherwise the change detection will get confused (ie return categories.filter(c => c.name === name) will not work). You should only return a new array when categories have been modified. There are plans to improve this by implementing a trackBy functionality.

@brandyscarney

This comment has been minimized.

Show comment
Hide comment
@brandyscarney

brandyscarney Oct 19, 2015

@vicb I actually started doing it that way but thought the NgIf would be the simplest way. Thanks for mentioning it though and letting me know not to return an array each time! I'll probably end up doing it that way. :)

brandyscarney commented Oct 19, 2015

@vicb I actually started doing it that way but thought the NgIf would be the simplest way. Thanks for mentioning it though and letting me know not to return an array each time! I'll probably end up doing it that way. :)

@lukedupin

This comment has been minimized.

Show comment
Hide comment
@lukedupin

lukedupin May 26, 2016

There is a simple work around. In my case, I had an array of elements, but I didn't always want to show them all.

Change this:

<select>
  <option *ngFor="#v of attr.values" *ngIf="v.selectable" [value]="v.id">{{ v.name }}</option>
</select>

To work like this:

<select>
  <template ngFor #v  [ngForOf]="attr.values">
    <option *ngIf="v.selectable" [value]="v.id" [selected]="attribute_selected[attr.id] == v.id">{{ v.name }}</option>
  </template>
</select>

lukedupin commented May 26, 2016

There is a simple work around. In my case, I had an array of elements, but I didn't always want to show them all.

Change this:

<select>
  <option *ngFor="#v of attr.values" *ngIf="v.selectable" [value]="v.id">{{ v.name }}</option>
</select>

To work like this:

<select>
  <template ngFor #v  [ngForOf]="attr.values">
    <option *ngIf="v.selectable" [value]="v.id" [selected]="attribute_selected[attr.id] == v.id">{{ v.name }}</option>
  </template>
</select>
@Ferhatos

This comment has been minimized.

Show comment
Hide comment
@Ferhatos

Ferhatos May 30, 2016

Thanks you @lukedupin, works for me as expected.

Ferhatos commented May 30, 2016

Thanks you @lukedupin, works for me as expected.

@marcj

This comment has been minimized.

Show comment
Hide comment
@marcj

marcj Feb 22, 2018

Another way to solve probably the most common one is to improve the ngFor syntax

<div *ngFor="let layer of input.children.all() if input"></div>

marcj commented Feb 22, 2018

Another way to solve probably the most common one is to improve the ngFor syntax

<div *ngFor="let layer of input.children.all() if input"></div>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment