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(ivy): support animation @triggers in templates #25849

Closed
wants to merge 1 commit into from

Conversation

matsko
Copy link
Contributor

@matsko matsko commented Sep 7, 2018

No description provided.

@mary-poppins
Copy link

You can preview 56f07e0 at https://pr25849-56f07e0.ngbuilds.io/.

if (rf & 2) {
$r3$.ɵelementAttribute(0, "@foo", $r3$.ɵbind(ctx.exp));
$r3$.ɵelementAttribute(1, "@bar", undefined);
$r3$.ɵelementAttribute(2, "@baz", undefined);
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this is desirable. If the value is constant (undefined) it shouldn't need to be reevaluated each update, should it? Also, because this doesn't use bind it will never be NO_CHANGE, in which case the attribute is thought to have changed during each change detection run.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, this test is not canonical, and will confuse people reading it. Can you make it canonical by binding to a value, which may be constant for the test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How's about just setting the undefined value to be NO_CHANGE?

Copy link
Member

Choose a reason for hiding this comment

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

@matsko It won't do a thing, then ;) Basically, constant attributes are usually applied for RenderFlags.Create by passing the constant attributes as third argument to elementStart.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK the instructions are gone.

Copy link
Contributor

@mhevery mhevery left a comment

Choose a reason for hiding this comment

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

Conditionally on the clean up comments.


const template = `
MyComponent.ngComponentDef = $r3$.ɵdefineComponent({
type: MyComponent,
Copy link
Contributor

Choose a reason for hiding this comment

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

you can use ... to elide irrelevant text and only keep animations.

MyComponent.ngComponentDef = $r3$.ɵdefineComponent({
 ...
  animations: [{name: "foo123"}, {name: "trigger123"}]
 ...
});

features: [$r3$.ɵPublicFeature],
consts: 0,
vars: 0,
template: function MyComponent_Template(rf, $ctx$) {
Copy link
Contributor

Choose a reason for hiding this comment

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

same here, elide irrelevant text.

if (rf & 2) {
$r3$.ɵelementAttribute(0, "@foo", $r3$.ɵbind(ctx.exp));
$r3$.ɵelementAttribute(1, "@bar", undefined);
$r3$.ɵelementAttribute(2, "@baz", undefined);
Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, this test is not canonical, and will confuse people reading it. Can you make it canonical by binding to a value, which may be constant for the test.

@mary-poppins
Copy link

You can preview bce5721 at https://pr25849-bce5721.ngbuilds.io/.

@mary-poppins
Copy link

You can preview 5d211ba at https://pr25849-5d211ba.ngbuilds.io/.

@matsko matsko added target: major This PR is targeted for the next major release action: merge The PR is ready for merge by the caretaker labels Sep 8, 2018
@IgorMinar IgorMinar closed this in e363388 Sep 10, 2018
FrederikSchlemmer pushed a commit to FrederikSchlemmer/angular that referenced this pull request Jan 3, 2019
@matsko matsko deleted the ivy_attr_triggers branch January 17, 2019 19:44
@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 14, 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 cla: yes target: major This PR is targeted for the next major release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants