FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

fix(animations): getAnimationStyle causes exceptions in older browsers by Serginho · Pull Request #29709 · angular/angular · GitHub

  1. angular
  2. angular

Repository navigation

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

Closed
Serginho wants to merge 1 commit into
angular:masterfrom
Serginho:fix_getAnimation_style
ConversationCommits (1)ChecksFiles changed
Closed

fix(animations): getAnimationStyle causes exceptions in older browsers#29709
Serginho wants to merge 1 commit into
angular:masterfrom
Serginho:fix_getAnimation_style

Conversation

Serginho commented Apr 4, 2019

Copy link
Copy Markdown
Contributor

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 requested a review from a team April 4, 2019 12:13
Serginho force-pushed the fix_getAnimation_style branch from 6a6e992 to 94d1810 Compare April 4, 2019 12:15
jasonaden added the area: animations legacy animations package only. Otherwise use area: core. label Apr 4, 2019
ngbot Bot added this to the needsTriage milestone Apr 4, 2019
BojanKogoj mentioned this pull request Nov 18, 2019
Closed
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 Bot requested a review from matsko February 28, 2020 22:48
kara removed the request for review from a team February 28, 2020 22:49

matsko commented Feb 28, 2020

Copy link
Copy Markdown
Contributor

@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 force-pushed the fix_getAnimation_style branch 2 times, most recently from fb2f0a8 to bd07373 Compare March 1, 2020 18:37

Serginho commented Mar 1, 2020

Copy link
Copy Markdown
Contributor Author

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

angular/packages/animations/browser/test/render/css_keyframes/element_animation_style_handler_spec.ts

Lines 66 to 68 in 5bc39f8

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 force-pushed the fix_getAnimation_style branch 3 times, most recently from 6f9210a to 9625229 Compare March 1, 2020 19:34
petebacondarwin requested review from petebacondarwin and removed request for matsko August 20, 2020 18:38
pullapprove Bot requested a review from crisbeto August 20, 2020 18:38
petebacondarwin approved these changes Aug 31, 2020
View reviewed changes

petebacondarwin left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

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

petebacondarwin commented Nov 10, 2020

Copy link
Copy Markdown
Contributor

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

Serginho force-pushed the fix_getAnimation_style branch from 9625229 to 8865d0c Compare November 16, 2020 23:25
Serginho force-pushed the fix_getAnimation_style branch from 8865d0c to ba35a0f Compare November 16, 2020 23:39

Serginho commented Nov 17, 2020

Copy link
Copy Markdown
Contributor Author

@petebacondarwin Sure.

Rebased and ready to merge.

petebacondarwin suggested changes Nov 17, 2020
View reviewed changes

petebacondarwin left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

Comment thread packages/animations/browser/test/render/css_keyframes/element_animation_style_handler_spec.ts Outdated
Serginho force-pushed the fix_getAnimation_style branch from ba35a0f to 2e26399 Compare November 17, 2020 10:40
petebacondarwin approved these changes Nov 17, 2020
View reviewed changes

petebacondarwin left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Great! Thanks.

AndrewKushnir commented Nov 18, 2020

Copy link
Copy Markdown
Contributor

Presubmit.

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
fix(animations): getAnimationStyle causes exceptions in older browsers (
cb1d77a 
#29709)

PR #29709 getAnimationStyle causes exceptions in older browsers

PR Close #29709

AndrewKushnir commented Nov 20, 2020

Copy link
Copy Markdown
Contributor

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

angular-automatic-lock-bot Bot commented Dec 21, 2020

Copy link
Copy Markdown

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 Bot locked and limited conversation to collaborators Dec 21, 2020
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

AndrewKushnir AndrewKushnir left review comments
crisbeto crisbeto approved these changes
+1 more reviewer
petebacondarwin petebacondarwin approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

action: merge The PR is ready for merge by the caretaker area: animations legacy animations package only. Otherwise use area: core. cla: yes target: patch This PR is targeted for the next patch release

Projects

None yet

Milestone

needsTriage

Development

Successfully merging this pull request may close these issues.

8 participants

Back | FazBrowse Home | New Git URL