Skip to content

Enable onAnimationModalInEnd callback #672

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

pichfl
Copy link
Contributor

@pichfl pichfl commented May 27, 2022

Closes #637

@pichfl pichfl added the enhancement New feature or request label May 27, 2022
@pichfl pichfl force-pushed the pichfl/animate-in branch from 17ec421 to 55caf58 Compare May 27, 2022 21:50
@pichfl
Copy link
Contributor Author

pichfl commented Jun 3, 2022

@nickschot and I went on a wild chase Even though this "fixes" the issue, it most likely does so by covering up the actual problem while also introducing a lot of moving parts. My initial assumption that animationend might not trigger when a user enables prefers-reduce-motion is wrong, all tested browsers still trigger the event even if the animation time is reduced to 0s. If there is a "valid" animation, it will trigger the event.

Quoting MDN (emphasis by me):

The animationend event is fired when a CSS Animation has completed. If the animation aborts before reaching completion, such as if the element is removed from the DOM or the animation is removed from the element, the animationend event is not fired.

We want to make sure, that janking the model component still triggers all necessary parts, but we shouldn't do so by introducing timers. Instead #686 is most likely one of the problems behind #665

I'm keeping this PR open for reference until #665 is resolved.

@pichfl pichfl force-pushed the pichfl/animate-in branch from 55caf58 to 62d6742 Compare October 14, 2022 18:48
@pichfl pichfl marked this pull request as ready for review October 15, 2022 22:38
@pichfl pichfl requested a review from nickschot October 15, 2022 22:38
@marcin-wicha
Copy link

Is anything blocking this currently? All of the referenced issues have been merged/closed.

@pichfl pichfl force-pushed the pichfl/animate-in branch 2 times, most recently from 9cda4c3 to 6919dfa Compare July 11, 2025 14:43
@pichfl pichfl force-pushed the pichfl/animate-in branch from 6919dfa to e52b635 Compare July 11, 2025 14:43
@pichfl
Copy link
Contributor Author

pichfl commented Jul 11, 2025

I've rebased and refactored to match the latest iteration, hoping to get this in with the next release.

Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Enables and documents a new onAnimationModalInEnd callback for opening animations, updates the modal component to invoke it, and adds an integration test to verify its invocation.

  • Introduce onAnimationModalInEnd option in modal API, component, and docs
  • Update EpmModal to expose and invoke the new callback
  • Add an integration test and adjust helpers/mocks to support animation callbacks

Reviewed Changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
packages/test-app/tests/integration/components/modal-test.js Add a test for onAnimationModalInEnd and onAnimationModalOutEnd callbacks
packages/test-app/tests/helpers/mocks.js Extend the default mock modal to support a click-to-close action
packages/test-app/tests/application/basics-test.js Remove obsolete sinon-based animation-start test
packages/ember-promise-modals/src/components/epm-modal.js Add an options getter and invoke the new onAnimationModalInEnd callback
packages/ember-promise-modals/src/components/epm-modal.hbs Update class binding to use options.className
packages/ember-promise-modals/README.md Document the new onAnimationModalInEnd hook
README.md Document the new onAnimationModalInEnd hook in the root README

@@ -1,4 +1,4 @@
import { render, settled, waitFor } from '@ember/test-helpers';
import { click, render, settled, waitFor } from '@ember/test-helpers';
Copy link
Preview

Copilot AI Jul 11, 2025

Choose a reason for hiding this comment

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

[nitpick] The click import is never used in this test. Remove it or add a click step to exercise the mock’s close button.

Copilot uses AI. Check for mistakes.

@@ -94,6 +94,8 @@ export default class EpmModal extends Component {
if (isOutAnimation) {
this.args.modal._remove();
}

this.options.onAnimationModalInEnd?.(animationName);
Copy link
Preview

Copilot AI Jul 11, 2025

Choose a reason for hiding this comment

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

This calls the "in end" callback for all animations, including the closing animation. It should only fire when the modal’s opening animation ends, and onAnimationModalOutEnd should be invoked for closing animations.

Suggested change
this.options.onAnimationModalInEnd?.(animationName);
if (isOutAnimation) {
this.options.onAnimationModalOutEnd?.(animationName);
} else {
this.options.onAnimationModalInEnd?.(animationName);
}

Copilot uses AI. Check for mistakes.

@@ -94,6 +94,8 @@ export default class EpmModal extends Component {
if (isOutAnimation) {
this.args.modal._remove();
}

this.options.onAnimationModalInEnd?.(animationName);
};

if (this.element) {
Copy link
Preview

Copilot AI Jul 11, 2025

Choose a reason for hiding this comment

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

[nitpick] An animationend listener is added but never removed. Consider cleaning it up (e.g., in a willDestroy hook) to prevent memory leaks when the component is destroyed.

Copilot uses AI. Check for mistakes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add an incoming animation end hook
2 participants