Skip to content
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

feat: ngIf now supports else; saves condition to local var #13297

Closed
wants to merge 4 commits into from

Conversation

mhevery
Copy link
Contributor

@mhevery mhevery commented Dec 8, 2016

NgIf syntax has been extended to support else clause to display template
when the condition is false. In addition the condition value can now
be stored in local variable, for later reuse. This is especially useful
when used with the async pipe.

Example:

<div *ngIf="userObservable | async; else loading; let user">
  Hello {{user.last}}, {{user.first}}!
</div>
<template #loading>Waiting...</template>

Closes #13061

@mhevery mhevery requested review from vicb and tbosch December 8, 2016 05:42
*
* # Showing alternative template using `else`
*
* At times it is important to display alternative template when the expression is falsy and inline
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is "At times it is important" really necessary in API docs ?

May use the then / else template terminology introduced earlier ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rewritten

*
* If the expression assigned to `ngIf` evaluates to a falsy value then the element
* is removed from the DOM, otherwise a clone of the element is reinserted into the DOM.
* # Using non-inlined `then` template
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

non-inlined -> reference ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think non-inlined makes more sense here.

*
* ### Example ([live demo](http://plnkr.co/edit/fe0kgemFBtmQOY31b4tw?p=preview)):
* Usually the `then` template is the inlined template of the `ngIf`, but it can be changed using
* a binding (just like `else`.) Because it is a binding, the `then` template can change over time.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because they are bindings, templates reference can be changed over time ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rewritten

* # Storing conditional result in a variable
*
* A common patter is that we need to show a set of properties from a same object, but if the
* object is undefined, the we have to use the safe-traversal-operator to guard against
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

, then (missing n)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

* ```
*
* There are several inefficiencies in the above example.
* - We create multiple subscriptions on the `userStream`, one for each `async` pipe.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

multiple -> 2

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rewritten

* - We create multiple subscriptions on the `userStream`, one for each `async` pipe.
* - We have to place the `async` pipe in parenthesis.
* - We have to use the safe-traversal-operator `.?` to access properties.
* - We can not display an alternative screen while waiting for the data to arrive asynchronously.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

move this one up (order by importance)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reordered

* - We have to use the safe-traversal-operator `.?` to access properties.
* - We can not display an alternative screen while waiting for the data to arrive asynchronously.
*
* The example below shows a better way to achieve a better result.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The better example ... :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rewritten

*
* Notice that:
* - We use only one `async` pipe and hence only one subscription gets created.
* - `ngIf` stores the result of the `userStream|async` in the `$implicit` location.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we need to mention implicit here (add a note below) or at all ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rewritten

* ### Syntax
*
* Simple form:
* - `<div *ngIf="condition">...</div>`
* - `<div template="ngIf condition">...</div>`
* - `<template [ngIf]="condition"><div>...</div></template>`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought we had decided to document this <ng-container *ngIf>.

Can you update as part of this PR ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Turns out that we can't do that because <ng-container #foo> is not supported. We need to fix that first nad than come back and change this.

* - `<div *ngIf="condition">...</div>`
* - `<div template="ngIf condition">...</div>`
* - `<template [ngIf]="condition"><div>...</div></template>`
*
* Form with an else block:
* - `<div *ngIf="condition; else elseBlock">...</div><template #elseBlock>...</template>`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove the leadin - ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

split on 2 (3 below) lines ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

this._hasView = false;
this._viewContainer.clear();
this._context.$implicit = condition;
this._updateView(condition);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this._updateView(); remove useless param and use the context

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

fixture = createTestComponent(template);

fixture.detectChanges();
expect(getDOM().getText(fixture.nativeElement)).toEqual('TRUE');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.toHaveText() doesn't work here ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changed (copy paste from existing test)

expect(getDOM().getText(fixture.nativeElement)).toEqual('FALSE');
}));

it('should support else', async(() => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

then + else ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

@@ -685,7 +685,7 @@ class TemplateParseVisitor implements html.Visitor {
}
elementProps.forEach(prop => {
this._reportError(
`Property binding ${prop.name} not used by any directive on an embedded template. Make sure that the property name is spelled correctly and all directives are listed in the "directives" section.`,
`Property binding ${prop.name} not used by any directive on an embedded template. Make sure that the property name is spelled correctly and all directives are listed in the "@NgModule.declarations".`,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or imported ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure I understand, what you mean by imported.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think he means: Property binding ${prop.name} not used by any directive on an embedded template. Make sure that the property name is spelled correctly and all directives are listed in the "@NgModule.declarations" or imported by the @NgModule

@@ -704,7 +704,7 @@ class TemplateParseVisitor implements html.Visitor {
events.forEach(event => {
if (isPresent(event.target) || !allDirectiveEvents.has(event.name)) {
this._reportError(
`Event binding ${event.fullName} not emitted by any directive on an embedded template. Make sure that the event name is spelled correctly and all directives are listed in the "directives" section.`,
`Event binding ${event.fullName} not emitted by any directive on an embedded template. Make sure that the event name is spelled correctly and all directives are listed in the "@NgModule.declarations".`,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same

# Running the examples

```
./build.sh # run only when framework code changes
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could you the command above the line and chg to # execute the following command only ...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

```

NOTE: sometimes the http server does not exits properly and it retans the `8001` port.
in Such a case you can use `lsof -i:8001` to see which process it is and then use `kill`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In such

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

@vicb
Copy link
Contributor

vicb commented Dec 8, 2016

very sweet !

LGTM after comments are addressed.

please aslo scope the commit messages by cmp.

@vicb vicb added area: common Issues related to APIs in the @angular/common package action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews pr_state: LGTM labels Dec 8, 2016
@mhevery mhevery removed the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Dec 8, 2016
@mhevery
Copy link
Contributor Author

mhevery commented Dec 8, 2016

@vicb please take another look.

@mhevery mhevery force-pushed the issue-13061 branch 2 times, most recently from 7f3b034 to ca9c028 Compare December 8, 2016 21:32
@vicb
Copy link
Contributor

vicb commented Dec 9, 2016

still failing CI

@vicb vicb added the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Dec 9, 2016
NgIf syntax has been extended to support else clause to display template
when the condition is false. In addition the condition value can now
be stored in local variable, for later reuse. This is especially useful
when used with the `async` pipe.

Example:

```
<div *ngIf="userObservable | async; else loading; let user">
  Hello {{user.last}}, {{user.first}}!
</div>
<template #loading>Waiting...</template>
```

Closes angular#13061
@mhevery
Copy link
Contributor Author

mhevery commented Dec 9, 2016

finally green, and all comments addressed.

./modules/@angular/examples/test.sh
```

NOTE: sometimes the http server does not exits properly and it retans the `8001` port.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does not exits -> does not exit
it retans -> it retains

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@souldreamer I'll fix when merging the PR. thanks

@vicb vicb added action: merge The PR is ready for merge by the caretaker and removed action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews labels Dec 9, 2016
@vicb vicb closed this in b4db73d Dec 9, 2016
@OndraZizka
Copy link

How do I know which version of Angular will have this? Is this documented anywhere? Thanks

@DzmitryShylovich
Copy link
Contributor

@OndraZizka
Copy link

OndraZizka commented Mar 1, 2017

@DzmitryShylovich, thanks, just found that too. I assume that means not going 2.x? Pity...

@DzmitryShylovich
Copy link
Contributor

@OndraZizka nope. this feature is 4.x only

@mhevery mhevery deleted the issue-13061 branch June 2, 2017 17:04
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 11, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker area: common Issues related to APIs in the @angular/common package cla: yes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ngIf improvements for Rx
6 participants