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 transition for bs-collapse #754

Merged
merged 1 commit into from
Feb 16, 2019
Merged

Fix transition for bs-collapse #754

merged 1 commit into from
Feb 16, 2019

Conversation

simonihmig
Copy link
Contributor

Fixes #629, introduced by #746.

@jelhan fyi. I tried to add some tests, asserting that the height increases/decreases during the transition. Which kinda worked, but were quite flaky, so skipped them for now.

@simonihmig simonihmig added the bug label Feb 16, 2019
@jelhan
Copy link
Contributor

jelhan commented Feb 16, 2019

Oh missed that one. Sorry.

I tried to add some tests, asserting that the height increases/decreases during the transition. Which kinda worked, but were quite flaky, so skipped them for now.

I guess related to hard-coded duration of 0ms in testing? https://github.com/kaliber5/ember-bootstrap/blob/v2.5.0/addon/utils/transition-end.js#L11-L13 That makes asserting interim states difficult, isn't it?

@simonihmig
Copy link
Contributor Author

I guess related to hard-coded duration of 0ms in testing? https://github.com/kaliber5/ember-bootstrap/blob/v2.5.0/addon/utils/transition-end.js#L11-L13 That makes asserting interim states difficult, isn't it?

Yeah, with that 0ms it obviously didn't work at all. But I played with a function to override this duration in tests, but even when setting this to the original 350ms, it wasn't working reliably. And it wasn't debuggable, as setting any breakpoint during the transition didn't allow to inspect what was going on, as the CSS transition wasn't stopped obviously. It was working like in 80% of the test runs, but failing without an obvious reason in the other ones... 🙄

@simonihmig
Copy link
Contributor Author

Oh missed that one. Sorry.

No problem at all! 🙂

@simonihmig simonihmig merged commit 8ebeb26 into master Feb 16, 2019
@simonihmig simonihmig deleted the collapse-transition branch February 16, 2019 23:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants