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

Apply visibility by clearing the property. #607

Merged
merged 1 commit into from
Jun 4, 2018

Conversation

brettburley
Copy link
Collaborator

@brettburley brettburley commented Jun 3, 2018

This PR is intended to address #606.

visibility: visible; and visibility: ; have different behaviors -- the former is visible even if a parent container has a visibility: hidden;. As a result, if there are nested animated containers, and a parent container uses the explode transition, there will be a ghost of the children that have visibility: visible applied since the transition relies on cloning the DOM.

Suggestions welcome on how this might be able to be tested.

@ef4
Copy link
Collaborator

ef4 commented Jun 3, 2018

Thanks. To test this, if you can add a route under /scenarios that demonstrates the problem interactively, I can help show how to write an automated test for it.

`visibility: visible;` and `visibility:;` have different behaviors --
the former is visible even if a parent container has a `visibility:
hidden;`. As a result, if there are nested animated containers, and a
parent container uses the explode transition, there will be a ghost of
the children that have `visibility: visible` applied since the
transition relies on cloning the DOM.
@brettburley
Copy link
Collaborator Author

Thanks @ef4, I totally missed the scenarios tests. I took a stab at writing one -- please take a look and provide any feedback or suggestions you have, thanks!

@ef4 ef4 merged commit 2415162 into ember-animation:master Jun 4, 2018
@ef4
Copy link
Collaborator

ef4 commented Jun 4, 2018

Looks nice, thanks again.

@ef4
Copy link
Collaborator

ef4 commented Jun 11, 2018

This PR may have caused a regression, haven't had a chance to dig into the details yet: #610

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

Successfully merging this pull request may close these issues.

2 participants