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(animation): Do not remove opacity for outgoing element #17422

Merged
merged 19 commits into from Mar 4, 2019

Conversation

2 participants
@liamdebeasi
Copy link
Member

commented Feb 7, 2019

Short description of what this resolves:

Fixes an issue where opacity is removed on an outgoing element right before it is removed from the DOM.

This only appears in Angular applications, but due to the way the lifecycles work there I think it may be best to just not bother removing the opacity (since the element is going to be trashed anyways).

This change is only for iOS (and desktop Safari in responsive design mode), and I did not notice any adverse effects when testing on the conference app as well as using vanilla ionic components.

Changes proposed in this pull request:

  • Remove true for outgoing animations, setting clearProperyAfterTransition to false

Ionic Version:

Fixes: #16988

@ionitron-bot ionitron-bot bot added the package: core label Feb 7, 2019

@liamdebeasi liamdebeasi requested a review from manucorporat Feb 7, 2019

@brandyscarney brandyscarney added this to In progress 🤺 in Ionic Core via automation Feb 8, 2019

@brandyscarney brandyscarney moved this from In progress 🤺 to Needs review 🤔 in Ionic Core Feb 8, 2019

brandyscarney and others added some commits Feb 8, 2019

@liamdebeasi liamdebeasi requested review from brandyscarney and removed request for manucorporat Feb 14, 2019

liamdebeasi and others added some commits Feb 14, 2019

liamdebeasi and others added some commits Feb 26, 2019

@brandyscarney

This comment has been minimized.

Copy link
Member

commented Feb 28, 2019

How can I reproduce this? I tried using the conference app in Safari but I don't see it.

@liamdebeasi

This comment has been minimized.

Copy link
Member Author

commented Mar 1, 2019

@brandyscarney If you navigate to a page and hit the back button, you should see the animation complete but then the header from the page you just navigated away from re-appear for a split second. Probably easiest to test on the latest master

@liamdebeasi

This comment has been minimized.

Copy link
Member Author

commented Mar 1, 2019

Also this repo shows it pretty well: https://github.com/lucaslevin/trainer (person who created the issue posted it)

@brandyscarney

This comment has been minimized.

Copy link
Member

commented Mar 1, 2019

@liamdebeasi Which test on master? I tried on the nav/basic one in Safari and didn't see it.

@liamdebeasi

This comment has been minimized.

Copy link
Member Author

commented Mar 1, 2019

Ah sorry, this issue only shows up in Angular applications. (despite the fix being in core)

@brandyscarney

This comment has been minimized.

Copy link
Member

commented Mar 4, 2019

I was able to reproduce this and confirm the fix in the test-app in the angular/test/ directory using the tabs test. Looks good to me. 👍

@liamdebeasi liamdebeasi merged commit ad20bd6 into master Mar 4, 2019

2 checks passed

build Workflow: build
Details
screenshot Screenshot
Details

Ionic Core automation moved this from Needs review 🤔 to Done 🎉 Mar 4, 2019

@brandyscarney brandyscarney deleted the fix-animation-flicker branch Mar 4, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.