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(collapse): restore previous transition logic #4388

Merged
merged 1 commit into from
Oct 17, 2022

Conversation

divdavem
Copy link
Member

@divdavem divdavem commented Sep 15, 2022

This PR restores the previous collapse transition logic that was changed with #4370.

Without this PR, the show and hide phases are inconsistent: the hide phase is from the full size of the container to 0px, and the show phase is from 0px to the scrollWidth/scrollHeight of the container and then there is a sudden change when the classes are changed at the end of the transition.

Using scrollWidth / scrollHeight does not feel right for me. We should use whatever size the container will have when changing the classes at the end of the transition, which is what measureCollapsingElementDimensionPx measures correctly. I have added style="max-width:300px;" on the container in the demo so that the animation matches the visible content.

Before submitting a pull request, please make sure you have at least performed the following:

  • read and followed the CONTRIBUTING.md and DEVELOPER.md guide.
  • built and tested the changes locally.
  • added/updated any applicable tests.
  • added/updated any applicable API documentation.
  • added/updated any applicable demos.

Copy link
Member

@maxokorokov maxokorokov left a comment

Choose a reason for hiding this comment

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

LGTM, thanks. It is better like this indeed

@maxokorokov maxokorokov merged commit ec372c3 into ng-bootstrap:master Oct 17, 2022
@maxokorokov maxokorokov added this to the 13.1.0 milestone Oct 17, 2022
@divdavem
Copy link
Member Author

@maxokorokov Thank you for reviewing and merging my PR!

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

Successfully merging this pull request may close these issues.

None yet

2 participants