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

fix(animations): getAnimationStyle causes exceptions in older browsers #29709

Closed
wants to merge 1 commit into from

Conversation

Serginho
Copy link
Contributor

@Serginho Serginho commented Apr 4, 2019

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • angular.io application / infrastructure changes
  • Other... Please describe:

What is the current behavior?

Issue Number: #24094

What is the new behavior?

Animation doesn't break in old browsers now

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

@Serginho Serginho requested a review from a team as a code owner April 4, 2019 12:13
@ngbot ngbot bot added this to the needsTriage milestone Apr 4, 2019
@kara kara added action: review The PR is still awaiting reviews from at least one requested reviewer target: patch This PR is targeted for the next patch release labels Feb 28, 2020
@pullapprove pullapprove bot requested a review from matsko February 28, 2020 22:48
@kara kara removed the request for review from a team February 28, 2020 22:49
@matsko
Copy link
Contributor

matsko commented Feb 28, 2020

@Serginho sorry this took us forever to get back to you on.

Could you add a test to this? You would need to export the function you modified and import it into https://github.com/angular/angular/blob/master/packages/animations/browser/test/render/css_keyframes/element_animation_style_handler_spec.ts

Then test by mocking out the a fake element a style property.

@Serginho Serginho force-pushed the fix_getAnimation_style branch 2 times, most recently from fb2f0a8 to bd07373 Compare March 1, 2020 18:37
@Serginho
Copy link
Contributor Author

Serginho commented Mar 1, 2020

@matsko I didn't do any additional test because getAnimationStyle is already tested, for example here:

assertStyle(element, 'animation-name', 'fooAnimation');
handler.apply();
assertStyle(element, 'animation-name', 'fooAnimation, barAnimation');

So I assume you want a testcase for this bug, and this is what I did in the updated commit.
The fix is squashed, formatted, and rebased to master.

Let me know if you need something else.

@Serginho Serginho force-pushed the fix_getAnimation_style branch 3 times, most recently from 6f9210a to 9625229 Compare March 1, 2020 19:34
@petebacondarwin petebacondarwin requested review from petebacondarwin and removed request for matsko August 20, 2020 18:38
@pullapprove pullapprove bot requested a review from crisbeto August 20, 2020 18:38
Copy link
Member

@petebacondarwin petebacondarwin left a comment

Choose a reason for hiding this comment

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

LGTM - please can you add some more information to the commit message body?

@petebacondarwin petebacondarwin added the action: presubmit The PR is in need of a google3 presubmit label Nov 10, 2020
@petebacondarwin
Copy link
Member

@Serginho - would you mind rebasing this PR on top of master. Then I will try to chase reviewers to get it merged.

Serginho added a commit to Serginho/angular that referenced this pull request Nov 16, 2020
PR angular#29709 getAnimationStyle causes exceptions in older browsers
@Serginho
Copy link
Contributor Author

@petebacondarwin Sure.

Rebased and ready to merge.

Copy link
Member

@petebacondarwin petebacondarwin left a comment

Choose a reason for hiding this comment

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

Thanks for rebasing @Serginho. Just one minor correction in the test.

PR angular#29709 getAnimationStyle causes exceptions in older browsers
Copy link
Member

@petebacondarwin petebacondarwin left a comment

Choose a reason for hiding this comment

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

Great! Thanks.

@AndrewKushnir
Copy link
Contributor

Presubmit.

@AndrewKushnir AndrewKushnir added action: merge The PR is ready for merge by the caretaker and removed action: review The PR is still awaiting reviews from at least one requested reviewer action: presubmit The PR is in need of a google3 presubmit labels Nov 20, 2020
AndrewKushnir pushed a commit that referenced this pull request Nov 20, 2020
#29709)

PR #29709 getAnimationStyle causes exceptions in older browsers

PR Close #29709
@AndrewKushnir
Copy link
Contributor

@Serginho this PR is now merged, thanks for contributing to Angular!

@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 Dec 21, 2020
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: animations cla: yes target: patch This PR is targeted for the next patch release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants