-
Notifications
You must be signed in to change notification settings - Fork 13.5k
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: remove css animations support for ionic animations #29123
Conversation
@@ -979,8 +860,6 @@ export const createAnimation = (animationId?: string): Animation => { | |||
if (supportsWebAnimations) { | |||
setAnimationStep(0); | |||
updateWebAnimation(); | |||
} else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add some comments where we still use the supportsWebAnimations
checks (or even a comment where the const is defined) that explains why we still use this support check?
@@ -950,21 +840,12 @@ export const createAnimation = (animationId?: string): Animation => { | |||
* element too quickly | |||
*/ | |||
raf(() => { | |||
clearCSSAnimationPlayState(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we still need the code above? It looks to be specific to CSS animations and display: none
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some level of fallback behavior is required to satisfy existing unit tests: https://github.com/ionic-team/ionic-framework/actions/runs/8333321414/job/22804541495.
I think we have two options:
- Have the minimal code to satisfy the unit test: 72a9662
- Remove the unit tests
I've done option 1 to reduce the number of changes. However I question the value of these tests at all. They would have only tested the fallback CSS behavior, not the web animation behavior.
If you are in favor, I'd propose removing those (can be either part of this PR or a separate ticket).
Discussed with the team and we decided that these tests do not provide substantial value as a unit test, since they will only be checking behavior that is already accounted for with other unit tests.
Issue number: Internal
What is the current behavior?
Ionic Framework provides a small utility wrapper around the Web Animations API. Historically not all browsers that Ionic Framework supported, had support for the Web Animations API. To offer backwards compatibility, Ionic Framework provided fallback behaviors for the different wrapped APIs.
What is the new behavior?
Does this introduce a breaking change?
All modern browsers support the Web Animations API today. If a developer needs to target an older browser that does not support Web Animations, they should either use a polyfill, or implement the fallback behavior themselves.
Other information