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.

Sign up for GitHub

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

Jump to bottom

fix(animations): properly support boolean-based transitions and state changes #19279

Closed
wants to merge 1 commit into from
Closed

fix(animations): properly support boolean-based transitions and state changes #19279

wants to merge 1 commit into from

Conversation

matsko
Copy link
Contributor

@matsko matsko commented Sep 19, 2017 •
edited

Closes #9396
Closes #12337

@matsko matsko added the target: patch This PR is targeted for the next patch release label Sep 19, 2017
@mary-poppins
Copy link

You can preview c809202 at https://pr19279-c809202.ngbuilds.io/.

@matsko matsko mentioned this pull request Sep 19, 2017
Closed
mhevery
mhevery suggested changes Sep 20, 2017
View reviewed changes
packages/animations/browser/src/render/transition_animation_engine.ts Outdated
return value != null ? value.toString() : null;
}
function normalizeTriggerValue(value: any): any {
return value != null ? value : null;
Copy link
Contributor

Choose a reason for hiding this comment

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

???

value != null ? value : null
should be same as:
value == null ? null : value
which should be same as
value

Am I missing something?

Copy link
Contributor

Choose a reason for hiding this comment

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

Note "not strict" comparison. It will convert undefined to null as well. I.e.

if value is undefined -> null
if value is null -> null
else -> value

Copy link
Contributor Author

Choose a reason for hiding this comment

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

add a comment.

packages/animations/browser/src/dsl/animation_transition_expr.ts
@@ -65,16 +65,27 @@ function parseAnimationAlias(alias: string, errors: string[]): string|Transition
}
}

const TRUE_BOOLEAN_VALUES = new Set<string>();
TRUE_BOOLEAN_VALUES.add('true');
TRUE_BOOLEAN_VALUES.add('1');
Copy link
Contributor

Choose a reason for hiding this comment

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

why is 1 as true? Usually everything which is not false is true.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

add documentation to the trigger docs

@mary-poppins
Copy link

You can preview f39d479 at https://pr19279-f39d479.ngbuilds.io/.

@mary-poppins
Copy link

You can preview 3295c6a at https://pr19279-3295c6a.ngbuilds.io/.

@matsko matsko added the action: merge The PR is ready for merge by the caretaker label Oct 5, 2017
@tbosch tbosch modified the milestone: Merge-queue Oct 6, 2017
@chuckjaz chuckjaz closed this in a8920eb Oct 6, 2017
chuckjaz added a commit to chuckjaz/angular that referenced this pull request Oct 7, 2017
@chuckjaz
Revert "fix(animations): properly support boolean-based transitions a…
6aa7cc1 
…nd state changes (angular#19279)"

This reverts commit a8920eb.
chuckjaz pushed a commit that referenced this pull request Oct 9, 2017
@matsko @chuckjaz
fix(animations): properly support boolean-based transitions and state…
c4704c8 
… changes (#19279)

Closes #9396
Closes #12337

PR Close #19279
@chuckjaz
Copy link
Contributor

@matsko This PR does not apply cleanly on 4.4.x. Can you create a 4.4.x PR with these changes?

@matsko matsko deleted the boolean_animation_triggers branch January 17, 2019 19:43
@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.
Reviewers

@devoto13 devoto13 devoto13 left review comments

@mhevery mhevery mhevery approved these changes

Assignees
No one assigned
Labels
action: merge The PR is ready for merge by the caretaker cla: yes target: patch This PR is targeted for the next patch release
Projects
None yet
Milestone
Merge-queue
7 participants
@matsko @mary-poppins @chuckjaz @mhevery @devoto13 @tbosch @googlebot