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: remove css animations support for ionic animations #29123

Merged
merged 12 commits into from
Mar 20, 2024

Conversation

sean-perkins
Copy link
Contributor

@sean-perkins sean-perkins commented Mar 7, 2024

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?

  • Removes the legacy CSS animations fallback behavior from the Web Animations API animation utility. Maintaining a few no-op behaviors for test environments.
  • Resolved a few internal type usages that were casting to any
  • Removed spec tests that were testing the fallback CSS animations behavior and/or already had test coverage from other unit tests.

Does this introduce a breaking change?

  • Yes
  • No

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

@sean-perkins sean-perkins changed the base branch from main to feature-8.0 March 7, 2024 15:39
@github-actions github-actions bot added the package: core @ionic/core package label Mar 7, 2024
@sean-perkins sean-perkins changed the title Sp/fw 5870 b feat: remove css animations support for ionic animations Mar 14, 2024
@sean-perkins sean-perkins marked this pull request as ready for review March 14, 2024 19:13
@sean-perkins sean-perkins requested review from liamdebeasi and a team as code owners March 14, 2024 19:13
@@ -979,8 +860,6 @@ export const createAnimation = (animationId?: string): Animation => {
if (supportsWebAnimations) {
setAnimationStep(0);
updateWebAnimation();
} else {
Copy link
Contributor

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();
Copy link
Contributor

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

Copy link
Contributor Author

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:

  1. Have the minimal code to satisfy the unit test: 72a9662
  2. 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).

@sean-perkins sean-perkins merged commit 892594d into feature-8.0 Mar 20, 2024
42 checks passed
@sean-perkins sean-perkins deleted the sp/FW-5870-b branch March 20, 2024 15:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package: core @ionic/core package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants