Skip to content
This repository has been archived by the owner on May 29, 2019. It is now read-only.

fix(collapse): avoid initial animation #5199

Closed
wants to merge 1 commit into from
Closed

fix(collapse): avoid initial animation #5199

wants to merge 1 commit into from

Conversation

bogdanalexe90
Copy link
Contributor

  • Ensure animation does not occur when first loaded

Fixes: #5192
Fixes: #4414

@Foxandxss
Copy link
Contributor

Can you create a plunker with the old broken behavior and another plunker on how it end with your fix?

The test stuff is really nice (that cleanup always is handy) but this would need another test if we end deciding in merging this.

Thank you!

@bogdanalexe90
Copy link
Contributor Author

I was able to reproduce this only with accordion plugin. For this reason i don't have any idea how to test the fix. Any suggestion are welcomed.

Before: http://plnkr.co/edit/l0Qc5rWfgCNhx4hU4JZx?p=preview
After: http://plnkr.co/edit/bp5PlTC5g6jjWDoLcleR?p=preview

@Foxandxss
Copy link
Contributor

What should I do / see there? I don't see any difference when loading.

@bogdanalexe90
Copy link
Contributor Author

ahh it was a false result. i will try to include some 3rd party libs. I'm experiencing a flickering a lot when using ui-select inside. Also i will try to have a look if i have some custom styles in my app.

I will try to create another plnkr soon.

@bogdanalexe90
Copy link
Contributor Author

I identified the following problems:

  1. An fixed height is set to the element
$animateCss(element, {
              addClass: 'in',
              easing: 'ease',
              to: { height: element[0].scrollHeight + 'px' }
            }).start()['finally'](expandDone);
  1. Some content arrives and element[0].scrollHeight changes
  2. The height is set to auto and the rest of the content is showed
element.removeClass('collapsing')
            .addClass('collapse')
            .css({height: 'auto'});

The problem (flickering) is only visible when you have some complex styles to render. I created a plnkr and you can see the describe behaviour with some breakpoints (on line 31 and line 44).

http://plnkr.co/edit/7dyZ3InIQ3gg69SmVAgT?p=preview

@bogdanalexe90
Copy link
Contributor Author

I had a look on #4414 . It seems it's the same problem, i created a plunker demonstrating the fix:

http://plnkr.co/edit/Y8Uf0KKDq2ubMxAa71dg?p=preview

@wesleycho
Copy link
Contributor

This needs a test still in order for us to accept it.

@@ -1,55 +1,76 @@
describe('collapse directive', function() {
describe('collapse directive', function () {
Copy link
Contributor

Choose a reason for hiding this comment

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

This whitespace change is unnecessary - this will need to all be changed back on merge.

@wesleycho
Copy link
Contributor

Minus all of the whitespace changes that should be undone, what are your thoughts on this @Foxandxss ?

@wesleycho wesleycho added this to the 1.0.4 milestone Jan 13, 2016
@wesleycho
Copy link
Contributor

Can you squash your commits into 1?

- Ensure animation does not occur when first loaded

Fixes: #5192
Fixes: #4414
@wesleycho wesleycho closed this in 57219aa Jan 15, 2016
@bogdanalexe90 bogdanalexe90 deleted the fix-collapse-initial-animation branch March 14, 2018 07:02
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants