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

Changed OnInit to AfterViewInit for accordion-item-body component. #726

Closed
wants to merge 1 commit into from

Conversation

Ballpin
Copy link

@Ballpin Ballpin commented Sep 19, 2018

This will give the component a chance to get the right height when
elements are being retrieved from REST API's or when other components
are involved.

This will give the component a chance to get the right height when
elements are being retrieved from REST API's or when other components
are invovled.
@codecov
Copy link

codecov bot commented Sep 19, 2018

Codecov Report

Merging #726 into master will not change coverage.
The diff coverage is 100%.

@@           Coverage Diff           @@
##           master     #726   +/-   ##
=======================================
  Coverage   76.01%   76.01%           
=======================================
  Files         192      192           
  Lines        5291     5291           
  Branches      399      399           
=======================================
  Hits         4022     4022           
  Misses       1184     1184           
  Partials       85       85
Impacted Files Coverage Δ
...ponents/accordion/accordion-item-body.component.ts 50% <100%> (ø) ⬆️

@nnixaa
Copy link
Collaborator

nnixaa commented Sep 19, 2018

Hi @Ballpin, thanks for the PR! I presume this won't work in case when the content is dynamically changed in runtime? Should we change it to onChanges?

@Ballpin
Copy link
Author

Ballpin commented Sep 19, 2018

Thank you @nnixaa for being of the devs creating nebular it's making wonders for me. :-)

I believe you are right on that part. For me it worked very well when a resolver was involved and prefetched the content. I could even use <nb user></nb-user> inside the accordion. A few hours later after submitting this PR, I noticed when you click at the "item-header" and expect the new content, it sticks to 35px height (or 1 rem to be exact).

Yes, change to onChanges if that works better.

@nnixaa
Copy link
Collaborator

nnixaa commented Sep 20, 2018

Hey @Ballpin, thanks for your kind words! Would you like to complete this PR by changing this calculation to onChanges so that you are able to check whether it is working correctly in your usecase?

@Ballpin
Copy link
Author

Ballpin commented Sep 20, 2018

Hi @nnixaa i tried to do it for several hours yesterday without success. Unfortunately i ended up with removing the animation instead for my use case.

If you succeed it would be awesome. :)

@nnixaa
Copy link
Collaborator

nnixaa commented Oct 16, 2018

Fixed in #882

@nnixaa nnixaa closed this Oct 16, 2018
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